Coverity scan results
Jason Evans
jasone at canonware.com
Sat Oct 11 21:51:50 PDT 2014
On Oct 11, 2014, at 8:35 PM, Eduardo Silva <edsiper at gmail.com> wrote:
> In Monkey[0] we use Jemalloc by default and when running a Coverity[1]
> static code analysis, it reported some issues, maybe some of them are
> false positives, but is better you check this:
Thanks for running Coverity on jemalloc. I received a Coverity analysis report from Pat Lynch last year for jemalloc 3.4.0 and fixed several bugs as a result. This time around, I don't see any bugs that haven't already been fixed in the dev branch, though I'd love to see Coverity results for the dev branch of jemalloc.
> ________________________________________________________________________________________________________
> *** CID 1017561: Macro compares unsigned to 0 (NO_EFFECT)
> /deps/jemalloc/src/jemalloc.c: 603 in malloc_conf_init()
> 597 SIZE_T_MAX, false)
> 598 CONF_HANDLE_SSIZE_T(opt_lg_dirty_mult,
> "lg_dirty_mult",
> 599 -1, (sizeof(size_t) << 3) - 1)
> 600 CONF_HANDLE_BOOL(opt_stats_print, "stats_print")
> 601 if (config_fill) {
> 602 CONF_HANDLE_BOOL(opt_junk, "junk")
>>>> CID 1017561: Macro compares unsigned to 0 (NO_EFFECT)
>>>> This less-than-zero comparison of an unsigned value is never true. "um < 0UL".
> 603
> CONF_HANDLE_SIZE_T(opt_quarantine, "quarantine",
> 604 0, SIZE_T_MAX, false)
> 605 CONF_HANDLE_BOOL(opt_redzone, "redzone")
> 606 CONF_HANDLE_BOOL(opt_zero, "zero")
> 607 }
> 608 if (config_utrace) {
This is protected by a comparison to 0, so there is no actual incorrect behavior, though Coverity continues to warn.
https://github.com/jemalloc/jemalloc/commit/e2985a23819670866c041ba07964099eeb9e0e07
> ________________________________________________________________________________________________________
> *** CID 1018162: Unchecked return value (CHECKED_RETURN)
> /deps/jemalloc/src/jemalloc.c: 1978 in jemalloc_constructor()
> 1972 */
> 1973 JEMALLOC_ATTR(constructor)
> 1974 static void
> 1975 jemalloc_constructor(void)
> 1976 {
> 1977
>>>> CID 1018162: Unchecked return value (CHECKED_RETURN)
>>>> Calling "malloc_init" without checking return value (as is done elsewhere 10 out of 11 times).
> 1978 malloc_init();
> 1979 }
> 1980
> 1981 #ifndef JEMALLOC_MUTEX_INIT_CB
> 1982 void
> 1983 jemalloc_prefork(void)
There are no actionable consequences to this call failing.
> ________________________________________________________________________________________________________
> *** CID 1019005: Uninitialized pointer read (UNINIT)
> /deps/jemalloc/src/ctl.c: 1344 in arena_i_dss_ctl()
> 1338 dss_prec_t dss_prec = dss_prec_limit;
> 1339
> 1340 malloc_mutex_lock(&ctl_mtx);
> 1341 WRITE(dss, const char *);
> 1342 match = false;
> 1343 for (i = 0; i < dss_prec_limit; i++) {
>>>> CID 1019005: Uninitialized pointer read (UNINIT)
>>>> Using uninitialized value "dss" when calling "strcmp".
> 1344 if (strcmp(dss_prec_names[i], dss) == 0) {
> 1345 dss_prec = i;
> 1346 match = true;
> 1347 break;
> 1348 }
> 1349 }
Fixed recently:
https://github.com/jemalloc/jemalloc/commit/586c8ede42d7d0545d36d9cbb0235fb39221ef3e
> ________________________________________________________________________________________________________
> *** CID 1022889: Missing unlock (LOCK)
> /deps/jemalloc/src/arena.c: 1848 in arena_dalloc_bin_run()
> 1842 arena_run_dalloc(arena, run, true, false);
> 1843 malloc_mutex_unlock(&arena->lock);
> 1844 /****************************/
> 1845 malloc_mutex_lock(&bin->lock);
> 1846 if (config_stats)
> 1847 bin->stats.curruns--;
>>>> CID 1022889: Missing unlock (LOCK)
>>>> Returning without unlocking "bin->lock.lock".
> 1848 }
> 1849
> 1850 static void
> 1851 arena_bin_lower_run(arena_t *arena, arena_chunk_t *chunk,
> arena_run_t *run,
> 1852 arena_bin_t *bin)
> 1853 {
This function actually drops bin->lock, does some other work, then reacquires bin->lock, so there is no issue.
> ________________________________________________________________________________________________________
> *** CID 1193408: Free of address-of expression (BAD_FREE)
> /deps/jemalloc/src/chunk_mmap.c: 110 in pages_trim()
> 104 {
> 105 size_t trailsize = alloc_size - leadsize - size;
> 106
> 107 if (leadsize != 0)
> 108 pages_unmap(addr, leadsize);
> 109 if (trailsize != 0)
>>>> CID 1193408: Free of address-of expression (BAD_FREE)
>>>> "pages_unmap" frees address offset from "ret".
> 110 pages_unmap((void *)((uintptr_t)ret +
> size), trailsize);
> 111 return (ret);
> 112 }
> 113 #endif
> 114 }
> 115
This is intentional.
> ________________________________________________________________________________________________________
> *** CID 1194963: Data race condition (MISSING_LOCK)
> /deps/jemalloc/src/prof.c: 430 in prof_ctx_init()
> 424 ctx->bt = bt;
> 425 ctx->lock = prof_ctx_mutex_choose();
> 426 /*
> 427 * Set nlimbo to 1, in order to avoid a race condition with
> 428 * prof_ctx_merge()/prof_ctx_destroy().
> 429 */
>>>> CID 1194963: Data race condition (MISSING_LOCK)
>>>> Accessing "ctx->nlimbo" without holding lock "malloc_mutex_s.lock". Elsewhere, "prof_ctx_s.nlimbo" is accessed with "malloc_mutex_s.lock" held 5 out of 7 times (2 of these accesses strongly imply that it is necessary).
> 430 ctx->nlimbo = 1;
> 431 ql_elm_new(ctx, dump_link);
> 432 memset(&ctx->cnt_merged, 0, sizeof(prof_cnt_t));
> 433 ql_new(&ctx->cnts_ql);
> 434 }
> 435
Lock-free data structures make this okay. Note that the lock-free data structures are gone in the dev branch, so the false positive list should be much shorter there.
> ________________________________________________________________________________________________________
> *** CID 1194964: Illegal address computation (OVERRUN)
> /deps/jemalloc/src/prof.c: 964 in prof_dump_maps()
> 958 if (prof_dump_flush(propagate_err) &&
> 959 propagate_err) {
> 960 ret = true;
> 961 goto label_return;
> 962 }
> 963 }
>>>> CID 1194964: Illegal address computation (OVERRUN)
>>>> "&prof_dump_buf[prof_dump_buf_end]" evaluates to an address that is at byte offset 65536 of an array of 1 bytes.
> 964 nread = read(mfd,
> &prof_dump_buf[prof_dump_buf_end],
> 965 PROF_DUMP_BUFSIZE - prof_dump_buf_end);
> 966 } while (nread > 0);
> 967 } else {
> 968 ret = true;
> 969 goto label_return;
This is in unreachable code due to configuration. If heap profiling were enabled, the buffer would be large enough to be safe.
> ________________________________________________________________________________________________________
> *** CID 1221142: Double lock (LOCK)
> /deps/jemalloc/src/prof.c: 1388 in je_prof_prefork()
> 1382 if (opt_prof) {
> 1383 unsigned i;
> 1384
> 1385 malloc_mutex_prefork(&bt2ctx_mtx);
> 1386 malloc_mutex_prefork(&prof_dump_seq_mtx);
> 1387 for (i = 0; i < PROF_NCTX_LOCKS; i++)
>>>> CID 1221142: Double lock (LOCK)
>>>> "je_malloc_mutex_prefork" locks "ctx_locks[i].lock" twice.
> 1388 malloc_mutex_prefork(&ctx_locks[i]);
> 1389 }
> 1390 }
> 1391
> 1392 void
> 1393 prof_postfork_parent(void)
i increments, so this code doesn't actually double lock the same mutex. It's interesting that Coverity detects iteration, but doesn't detect that the loop variable discriminates mutexes.
> ________________________________________________________________________________________________________
> [... numerous warnings due to lock-free data structures ...]
> ________________________________________________________________________________________________________
> *** CID 1221208: Wrong sizeof argument (SIZEOF_MISMATCH)
> /deps/jemalloc/src/stats.c: 410 in je_stats_print()
> 404
> 405 malloc_cprintf(write_cb, cbopaque, "CPUs: %u\n", ncpus);
> 406
> 407 CTL_GET("arenas.narenas", &uv, unsigned);
> 408 malloc_cprintf(write_cb, cbopaque, "Arenas: %u\n", uv);
> 409
>>>> CID 1221208: Wrong sizeof argument (SIZEOF_MISMATCH)
>>>> Passing argument "cbopaque" of type "void *" and argument "8UL /* sizeof (void *) */" to function "je_malloc_cprintf" is suspicious.
> 410 malloc_cprintf(write_cb, cbopaque, "Pointer
> size: %zu\n",
> 411 sizeof(void *));
> 412
> 413 CTL_GET("arenas.quantum", &sv, size_t);
> 414 malloc_cprintf(write_cb, cbopaque, "Quantum
> size: %zu\n", sv);
> 415
This appears to be tripping up an inapplicable detector in Coverity.
More information about the jemalloc-discuss
mailing list