Loading...

XML

Word

Printable

Type: Patch request
Resolution: Fixed
Priority: Trivial
Fix Version/s: 4.0.17rc1, 4.4.5rc1, 5.0.0alpha1, 5.0 (plan)
Affects Version/s: 4.2.8
Component/s: Agent (G)
Labels:
- agent
- patch
Environment:
Ubuntu 18.04.2 LTS

Sprint:
Sprint 59 (Dec 2019), Sprint 60 (Jan 2020)
Story Points:
0.25

I found memory leak of zabbix-agentd(confirmed ver: 4.2.5, and 4.2.8).
then I searched root cause of leak by valgrind.

The result is following.

==8900== Conditional jump or move depends on uninitialised value(s)
==8900==    at 0x4C35E6F: bcmp (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8900==    by 0x154E28: zbx_read (file.c:116)
==8900==    by 0x1308F2: VFS_FILE_CONTENTS (file.c:211)
==8900==    by 0x123A5A: process (sysinfo.c:632)
==8900==    by 0x121A71: process_listener (listener.c:57)
==8900==    by 0x121A71: listener_thread (listener.c:145)
==8900==    by 0x13BEA6: zbx_thread_start (threads.c:136)
==8900==    by 0x118425: MAIN_ZABBIX_ENTRY (zabbix_agentd.c:1018)
==8900==    by 0x13CC4C: daemon_start (daemon.c:387)
==8900==    by 0x116C7C: main (zabbix_agentd.c:1267)
==8900==
==8900== Conditional jump or move depends on uninitialised value(s)
==8900==    at 0x154E2B: zbx_read (file.c:116)
==8900==    by 0x1308F2: VFS_FILE_CONTENTS (file.c:211)
==8900==    by 0x123A5A: process (sysinfo.c:632)
==8900==    by 0x121A71: process_listener (listener.c:57)
==8900==    by 0x121A71: listener_thread (listener.c:145)
==8900==    by 0x13BEA6: zbx_thread_start (threads.c:136)
==8900==    by 0x118425: MAIN_ZABBIX_ENTRY (zabbix_agentd.c:1018)
==8900==    by 0x13CC4C: daemon_start (daemon.c:387)
==8900==    by 0x116C7C: main (zabbix_agentd.c:1267)
==8900==
==8900== Conditional jump or move depends on uninitialised value(s)
==8900==    at 0x4C35E6F: bcmp (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8900==    by 0x154DFF: zbx_read (file.c:122)
==8900==    by 0x1308F2: VFS_FILE_CONTENTS (file.c:211)
==8900==    by 0x123A5A: process (sysinfo.c:632)
==8900==    by 0x121A71: process_listener (listener.c:57)
==8900==    by 0x121A71: listener_thread (listener.c:145)
==8900==    by 0x13BEA6: zbx_thread_start (threads.c:136)
==8900==    by 0x118425: MAIN_ZABBIX_ENTRY (zabbix_agentd.c:1018)
==8900==    by 0x13CC4C: daemon_start (daemon.c:387)
==8900==    by 0x116C7C: main (zabbix_agentd.c:1267)
==8900==
==8900== Conditional jump or move depends on uninitialised value(s)
==8900==    at 0x154E02: zbx_read (file.c:122)
==8900==    by 0x1308F2: VFS_FILE_CONTENTS (file.c:211)
==8900==    by 0x123A5A: process (sysinfo.c:632)
==8900==    by 0x121A71: process_listener (listener.c:57)
==8900==    by 0x121A71: listener_thread (listener.c:145)
==8900==    by 0x13BEA6: zbx_thread_start (threads.c:136)
==8900==    by 0x118425: MAIN_ZABBIX_ENTRY (zabbix_agentd.c:1018)
==8900==    by 0x13CC4C: daemon_start (daemon.c:387)
==8900==    by 0x116C7C: main (zabbix_agentd.c:1267)
==8900==
==8900== Conditional jump or move depends on uninitialised value(s)
==8900==    at 0x4C32CF9: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8900==    by 0x14ED0D: zbx_strcpy_alloc (str.c:353)
==8900==    by 0x130964: VFS_FILE_CONTENTS (file.c:226)
==8900==    by 0x123A5A: process (sysinfo.c:632)
==8900==    by 0x121A71: process_listener (listener.c:57)
==8900==    by 0x121A71: listener_thread (listener.c:145)
==8900==    by 0x13BEA6: zbx_thread_start (threads.c:136)
==8900==    by 0x118425: MAIN_ZABBIX_ENTRY (zabbix_agentd.c:1018)
==8900==    by 0x13CC4C: daemon_start (daemon.c:387)
==8900==    by 0x116C7C: main (zabbix_agentd.c:1267)
==8900==
==8900== Conditional jump or move depends on uninitialised value(s)
==8900==    at 0x4C32D08: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8900==    by 0x14ED0D: zbx_strcpy_alloc (str.c:353)
==8900==    by 0x130964: VFS_FILE_CONTENTS (file.c:226)
==8900==    by 0x123A5A: process (sysinfo.c:632)
==8900==    by 0x121A71: process_listener (listener.c:57)
==8900==    by 0x121A71: listener_thread (listener.c:145)
==8900==    by 0x13BEA6: zbx_thread_start (threads.c:136)
==8900==    by 0x118425: MAIN_ZABBIX_ENTRY (zabbix_agentd.c:1018)
==8900==    by 0x13CC4C: daemon_start (daemon.c:387)
==8900==    by 0x116C7C: main (zabbix_agentd.c:1267)
==8900==
==8900== Conditional jump or move depends on uninitialised value(s)
==8900==    at 0x14EB79: zbx_strncpy_alloc (str.c:322)
==8900==    by 0x130964: VFS_FILE_CONTENTS (file.c:226)
==8900==    by 0x123A5A: process (sysinfo.c:632)
==8900==    by 0x121A71: process_listener (listener.c:57)
==8900==    by 0x121A71: listener_thread (listener.c:145)
==8900==    by 0x13BEA6: zbx_thread_start (threads.c:136)
==8900==    by 0x118425: MAIN_ZABBIX_ENTRY (zabbix_agentd.c:1018)
==8900==    by 0x13CC4C: daemon_start (daemon.c:387)
==8900==    by 0x116C7C: main (zabbix_agentd.c:1267)
==8900==
==8900== Conditional jump or move depends on uninitialised value(s)
==8900==    at 0x14EB8B: zbx_strncpy_alloc (str.c:322)
==8900==    by 0x130964: VFS_FILE_CONTENTS (file.c:226)
==8900==    by 0x123A5A: process (sysinfo.c:632)
==8900==    by 0x121A71: process_listener (listener.c:57)
==8900==    by 0x121A71: listener_thread (listener.c:145)
==8900==    by 0x13BEA6: zbx_thread_start (threads.c:136)
==8900==    by 0x118425: MAIN_ZABBIX_ENTRY (zabbix_agentd.c:1018)
==8900==    by 0x13CC4C: daemon_start (daemon.c:387)
==8900==    by 0x116C7C: main (zabbix_agentd.c:1267)
==8900==
==9141==
==9141== HEAP SUMMARY:
==9141==     in use at exit: 11,263 bytes in 185 blocks
==9141==   total heap usage: 8,404 allocs, 8,219 frees, 5,982,142 bytes allocated
==9141==
==9141==
==9141== HEAP SUMMARY:
==9141==     in use at exit: 11,263 bytes in 185 blocks
==9141==   total heap usage: 8,404 allocs, 8,219 frees, 5,982,142 bytes allocated
==9141==
==9141== LEAK SUMMARY:
==9141==    definitely lost: 0 bytes in 0 blocks
==9141==    indirectly lost: 0 bytes in 0 blocks
==9141==      possibly lost: 0 bytes in 0 blocks
==9141==    still reachable: 11,263 bytes in 185 blocks
==9141==         suppressed: 0 bytes in 0 blocks
==9141== Reachable blocks (those to which a pointer was found) are not shown.
==9141== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==9141==
==9141== For counts of detected and suppressed errors, rerun with: -v
==9141== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==9142==
==9142== HEAP SUMMARY:
==9142==     in use at exit: 77,054 bytes in 187 blocks
==9142==   total heap usage: 30,124 allocs, 29,937 frees, 6,043,840 bytes allocated
==9142==
==9142== 65,536 bytes in 1 blocks are definitely lost in loss record 37 of 37
==9142==    at 0x4C31D2F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9142==    by 0x14A46A: zbx_realloc2 (misc.c:548)
==9142==    by 0x14EBE5: zbx_strncpy_alloc (str.c:319)
==9142==    by 0x130964: VFS_FILE_CONTENTS (file.c:226)
==9142==    by 0x123A5A: process (sysinfo.c:632)
==9142==    by 0x121A71: process_listener (listener.c:57)
==9142==    by 0x121A71: listener_thread (listener.c:145)
==9142==    by 0x13BEA6: zbx_thread_start (threads.c:136)
==9142==    by 0x118425: MAIN_ZABBIX_ENTRY (zabbix_agentd.c:1018)
==9142==    by 0x13CC4C: daemon_start (daemon.c:387)
==9142==    by 0x116C7C: main (zabbix_agentd.c:1267)

[1]. Unstable statement
First of all, I read 'zbx_realloc2' in 'src/libs/zbxcommon/misc.c'
in that block, I found following unstable expression.

void    *zbx_realloc2(const char *filename, int line, void *old, size_t size)
{
        int     max_attempts;
        void    *ptr = NULL;

        for (
                max_attempts = 10, size = MAX(size, 1);
                0 < max_attempts && NULL == ptr;
                ptr = realloc(old, size), max_attempts--
        );

        if (NULL != ptr)
                return ptr;

        zabbix_log(LOG_LEVEL_CRIT, "[file:%s,line:%d] zbx_realloc: out of memory. Requested " ZBX_FS_SIZE_T " bytes.",
                        filename, line, (zbx_fs_size_t)size);

        exit(EXIT_FAILURE);
}

in this FOR statement, I think that the successful allocation might be executed 2 times.

first successful allocation was occurred, the prt has not NLL value. then, at the start of the next loop, '0 < max_attempts && NULL == ptr' will be executed. the result of this will be FALST because 'NULL == ptr' will be FALSE. then, but we tend to think exit this FOR loop, post-execution formula will be executed. so 'ptr = realloc(old, size), max_attempts-- ' statement called again.
in this operation, we may lost the address of 1st re-allocation because the 'ptr' will be rewrote by 2nd operation.
In addition, zbx_realloc() function has insufficient error handling.
if we obtain NULL in ptr after realloc() operation, this function may be exit without free up memory of 'old'. so we should insert 'free(old);' statement inerror handing operation.

the similar unstable statements ware seen in

void    *zbx_malloc2(const char *filename, int line, void *old, size_t size);
void    *zbx_calloc2(const char *filename, int line, void *old, size_t nmemb, size_t size);
char    *zbx_strdup2(const char *filename, int line, char *old, const char *str);

I think that statements should be fixed like below

 jp7fkf@lab1:~/zabbix-4.2.8$ diff -u -r zabbix-4.2.8/src/libs/zbxcommon/misc.c zabbix-4.2.8_fkf/src/libs/zbxcommon/misc.c
--- zabbix-4.2.8/src/libs/zbxcommon/misc.c  2019-10-28 11:12:26.000000000 +0000
+++ zabbix-4.2.8_fkf/src/libs/zbxcommon/misc.c  2019-12-03 05:41:17.192000000 +0000
@@ -476,8 +476,9 @@
  for (
    max_attempts = 10, nmemb = MAX(nmemb, 1), size = MAX(size, 1);
    0 < max_attempts && NULL == ptr;
-   ptr = calloc(nmemb, size), max_attempts--
- );
+   max_attempts--
+ )
+   ptr = calloc(nmemb, size);

  if (NULL != ptr)
    return ptr;
@@ -515,8 +516,9 @@
  for (
    max_attempts = 10, size = MAX(size, 1);
    0 < max_attempts && NULL == ptr;
-   ptr = malloc(size), max_attempts--
- );
+   max_attempts--
+ )
+   ptr = malloc(size);

  if (NULL != ptr)
    return ptr;
@@ -547,12 +549,15 @@
  for (
    max_attempts = 10, size = MAX(size, 1);
    0 < max_attempts && NULL == ptr;
-   ptr = realloc(old, size), max_attempts--
- );
+   max_attempts--
+ )
+   ptr = realloc(old, size);

  if (NULL != ptr)
    return ptr;

+ zbx_free(old);
+
  zabbix_log(LOG_LEVEL_CRIT, "[file:%s,line:%d] zbx_realloc: out of memory. Requested " ZBX_FS_SIZE_T " bytes.",
      filename, line, (zbx_fs_size_t)size);

@@ -566,8 +571,8 @@

  zbx_free(old);

- for (retry = 10; 0 < retry && NULL == ptr; ptr = strdup(str), retry--)
-   ;
+ for (retry = 10; 0 < retry && NULL == ptr; retry--)
+   ptr = strdup(str);

  if (NULL != ptr)
    return ptr;

[2]zbx_read() may return unexpected value.

I found the following log when I execute modified 'zbx_read()'

19207:20191202:093521.406 zbx_read: read() returned: -1 nbytes
19207:20191202:093521.406 zbx_read: zbx_read() returned: 66 nbytes

modified zbx_read:

int     zbx_read(int fd, char *buf, size_t count, const char *encoding)
{
        size_t          i, szbyte, nbytes;
        const char      *cr, *lf;
        zbx_offset_t    offset;

        if ((zbx_offset_t)-1 == (offset = zbx_lseek(fd, 0, SEEK_CUR)))
                return -1;

        if (0 >= (nbytes = read(fd, buf, (unsigned int)count)))
        {
                zabbix_log(LOG_LEVEL_CRIT, "zbx_read: read() returned: %d nbytes", (int)nbytes);
                return (int)nbytes;
        }

        find_cr_lf_szbyte(encoding, &cr, &lf, &szbyte);

        for (i = 0; i <= nbytes - szbyte; i += szbyte)
        {
                if (0 == memcmp(&buf[i], lf, szbyte))   /* LF (Unix) */
                {
                        i += szbyte;
                        break;
                }

                if (0 == memcmp(&buf[i], cr, szbyte))   /* CR (Mac) */
                {
                        /* CR+LF (Windows) ? */
                        if (i < nbytes - szbyte && 0 == memcmp(&buf[i + szbyte], lf, szbyte))
                                i += szbyte;

                        i += szbyte;
                        break;
                }
        }

        if ((zbx_offset_t)-1 == zbx_lseek(fd, offset + (zbx_offset_t)i, SEEK_SET))
                return -1;
                
        zabbix_log(LOG_LEVEL_CRIT, "zbx_read: zbx_read() returned: %d nbytes", (int)nbytes);
        return (int)i;
}

so I think the 'if (0 >= (nbytes = read(fd, buf, (unsigned int)count)))' statement might insufficient of cast.

then I fixed it like below

jp7fkf@lab1:~/zabbix-4.2.8$ diff -u -r zabbix-4.2.8/src/libs/zbxcommon/file.c zabbix-4.2.8_fkf/src/libs/zbxcommon/file.c
--- zabbix-4.2.8/src/libs/zbxcommon/file.c	2019-10-28 11:12:26.000000000 +0000
+++ zabbix-4.2.8_fkf/src/libs/zbxcommon/file.c	2019-12-03 05:35:48.848000000 +0000
@@ -106,7 +106,7 @@
 	if ((zbx_offset_t)-1 == (offset = zbx_lseek(fd, 0, SEEK_CUR)))
 		return -1;

-	if (0 >= (nbytes = read(fd, buf, (unsigned int)count)))
+	if (0 >= (int)(nbytes = read(fd, buf, (unsigned int)count)))
 		return (int)nbytes;

 	find_cr_lf_szbyte(encoding, &cr, &lf, &szbyte);

after apply 2 fixes above, the memory leak of zabbix-agentd are resolved.

[SUMMARY]

I propose 2 bug-fixes in the zabbix-agentd source.

I want to merge the proposed fix code above to the master branch of zabbix-git, and we provide fixed binary as quick as we can.

Best regurds.

=====
Yudai Hashimoto(JP7FKF)
https://jp7fkf.dev/

Assignee:: Martins Abele

Reporter:: Yudai Hashimoto

Team:: Team A

Votes:: 0 Vote for this issue

Watchers:: 7 Start watching this issue

Created:: 2019 Dec 04 15:28

Updated:: 2024 Apr 10 16:51

Resolved:: 2020 Jan 17 09:31

Details

Description

Attachments

Activity

People

Dates