Uploaded image for project: 'ZABBIX BUGS AND ISSUES'
  1. ZABBIX BUGS AND ISSUES
  2. ZBX-17036

fix: insufficient cast in zbx_read() and unstable statement

    XMLWordPrintable

    Details

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

      Description

      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/

        Attachments

          Activity

            People

            Assignee:
            mabele Martins Abele
            Reporter:
            jp7fkf Yudai Hashimoto
            Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: