Coverity scan results for jemalloc-5.1.0

[vashirov]

We use jemalloc in our project and run Coverity on a regular basis. It reported some issues in jemalloc code, I'm not sure if they are false positives or not, so please take a look.

jemalloc-5.1.0/src/arena.c:

288 static void
289 arena_large_malloc_stats_update(tsdn_t *tsdn, arena_t *arena, size_t usize) {
290         szind_t index, hindex;
291 
>>      1. Condition 0L /* !!!config_stats */, taking false branch.
292         cassert(config_stats);
293 
>>      2. Condition usize < (16384UL /* (size_t)1 << 14 */), taking false branch.
294         if (usize < LARGE_MINCLASS) {
295                 usize = LARGE_MINCLASS;
296         }
>>      3. return_constant: Function call sz_size2index(usize) returns 232.
>>      4. assignment: Assigning: index = sz_size2index(usize). The value of index is now 232.
297         index = sz_size2index(usize);
>>      5. Condition index >= 36, taking true branch.
>>      6. assignment: Assigning: hindex = (index >= 36U) ? index - 36U : 0U. The value of hindex is now 196.
298         hindex = (index >= NBINS) ? index - NBINS : 0;
299 
>>      
>> CID 17477 (#1 of 1): Out-of-bounds read (OVERRUN)
>> 7. overrun-local: Overrunning array arena->stats.lstats of 196 32-byte elements at element index 196 (byte offset 6272) using index hindex (which evaluates to 196).
300         arena_stats_add_u64(tsdn, &arena->stats,
301             &arena->stats.lstats[hindex].nmalloc, 1);
302 }

304 static void
305 arena_large_dalloc_stats_update(tsdn_t *tsdn, arena_t *arena, size_t usize) {
306         szind_t index, hindex;
307 
>>      1. Condition 0L /* !!!config_stats */, taking false branch.
308         cassert(config_stats);
309 
>>      2. Condition usize < (16384UL /* (size_t)1 << 14 */), taking false branch.
310         if (usize < LARGE_MINCLASS) {
311                 usize = LARGE_MINCLASS;
312         }
>>      3. return_constant: Function call sz_size2index(usize) returns 232.
>>      4. assignment: Assigning: index = sz_size2index(usize). The value of index is now 232.
313         index = sz_size2index(usize);
>>      5. Condition index >= 36, taking true branch.
>>      6. assignment: Assigning: hindex = (index >= 36U) ? index - 36U : 0U. The value of hindex is now 196.
314         hindex = (index >= NBINS) ? index - NBINS : 0;
315 
>> 
>> CID 17476 (#1 of 1): Out-of-bounds read (OVERRUN)
>> 7. overrun-local: Overrunning array arena->stats.lstats of 196 32-byte elements at element index 196 (byte offset 6272) using index hindex (which evaluates to 196).
316         arena_stats_add_u64(tsdn, &arena->stats,
317             &arena->stats.lstats[hindex].ndalloc, 1);
318 }

jemalloc-5.1.0/src/background_thread.c:

>> CID 17475 (#1 of 1): Side effect in assertion (ASSERT_SIDE_EFFECT)
>> assert_side_effect: Argument n_search++ of assert() has a side effect. The containing function might work differently in a non-debug build.
191                assert(n_search++ < lg_floor(SMOOTHSTEP_NSTEPS) + 1);

jemalloc-5.1.0/src/jemalloc.c:

>> 
>> CID 13349 (#1 of 1): Macro compares unsigned to 0 (NO_EFFECT)
>> unsigned_compare: This less-than-zero comparison of an unsigned value is never true. um < 0UL.
1144                        CONF_HANDLE_SIZE_T(opt_lg_extent_max_active_fit,
1145                            "lg_extent_max_active_fit", 0,
1146                            (sizeof(size_t) << 3), yes, yes, false)

3180 #ifndef JEMALLOC_JET
3181 JEMALLOC_ATTR(constructor)
3182 static void
3183 jemalloc_constructor(void) {
>> 
>> CID 13340 (#1 of 1): Unchecked return value (CHECKED_RETURN)
>> 1. check_return: Calling malloc_init without checking return value (as is done elsewhere 5 out of 6 times).
3184         malloc_init();
3185 }
3186 #endif

jemalloc-5.1.0/src/pages.c:

99  os_pages_trim(void *addr, size_t alloc_size, size_t leadsize, size_t size,
100     bool *commit) {
101         void *ret = (void *)((uintptr_t)addr + leadsize);
102 
103         assert(alloc_size >= leadsize + size);
104 #ifdef _WIN32
105         os_pages_unmap(addr, alloc_size);
106         void *new_addr = os_pages_map(ret, size, PAGE, commit);
107         if (new_addr == ret) {
108                 return ret;
109         }
110         if (new_addr != NULL) {
111                 os_pages_unmap(new_addr, size);
112         }
113         return NULL;
114 #else
115         size_t trailsize = alloc_size - leadsize - size;
116 
117         if (leadsize != 0) {
118                 os_pages_unmap(addr, leadsize);
119         }
120         if (trailsize != 0) {
>> 
>> CID  17474 (#1 of 1): Free of address-of expression (BAD_FREE)
>> off set_free: os_pages_unmap frees address offset from ret. [show details]
121                 os_pages_unmap((void *)((uintptr_t)ret + size), trailsize);
122         }
123         return ret;
124 #endif
125 }

515 static void
516 init_thp_state(void) {
>>      1. Condition 0 /* !have_madvise_huge */, taking false branch.
517         if (!have_madvise_huge) {
518                 if (metadata_thp_enabled() && opt_abort) {
519                         malloc_write("<jemalloc>: no MADV_HUGEPAGE support\n");
520                         abort();
521                 }
522                 goto label_error;
523         }
524 
525         static const char sys_state_madvise[] = "always [madvise] never\n";
526         static const char sys_state_always[] = "[always] madvise never\n";
527         static const char sys_state_never[] = "always madvise [never]\n";
528         char buf[sizeof(sys_state_madvise)];
529 
530 #if defined(JEMALLOC_USE_SYSCALL) && defined(SYS_open)
531         int fd = (int)syscall(SYS_open,
532             "/sys/kernel/mm/transparent_hugepage/enabled", O_RDONLY);
533 #else
534         int fd = open("/sys/kernel/mm/transparent_hugepage/enabled", O_RDONLY);
535 #endif
>>      2. Condition fd == -1, taking false branch.
536         if (fd == -1) {
537                 goto label_error;
538         }
539 
>>      3. negative_return_fn: Function malloc_read_fd(fd, &buf, 24UL) returns a negative number. [show details]
>>      4. var_assign: Assigning: signed variable nread = malloc_read_fd.
540         ssize_t nread = malloc_read_fd(fd, &buf, sizeof(buf));
541 #if defined(JEMALLOC_USE_SYSCALL) && defined(SYS_close)
542         syscall(SYS_close, fd);
543 #else
544         close(fd);
545 #endif
546 
>>      
>> CID  18356 (#1 of 1): Argument cannot be negative (NEGATIVE_RETURNS)
>> 5.  negative_returns: (size_t)nread is passed to a parameter that cannot be negative.
547         if (strncmp(buf, sys_state_madvise, (size_t)nread) == 0) {
548                 init_system_thp_mode = thp_mode_default;
549         } else if (strncmp(buf, sys_state_always, (size_t)nread) == 0) {
550                 init_system_thp_mode = thp_mode_always;
551         } else if (strncmp(buf, sys_state_never, (size_t)nread) == 0) {
552                 init_system_thp_mode = thp_mode_never;
553         } else {
554                 goto label_error;
555         }
556         return;
557 label_error:
558         opt_thp = init_system_thp_mode = thp_mode_not_supported;
559 }

[djwatson]

Thanks! Only the pages.c one looks like a false positive, the rest could use fixing (although they mostly look harmless)

[aditya7fb]

jemalloc-5.1.0/src/background_thread.c:

is harmless as n_search is only used inside assert. Although it will be a good idea to move it out of assert to prevent future bugs.

[aditya7fb]

jemalloc-5.1.0/src/pages.c:init_thp_state

fixed. https://github.com/jemalloc/jemalloc/commit/856319dc8a3d15c3eddf83d106e01e6f63c349a7