1325082 - Coverity report of missing lock in jemalloc update

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Note: MALLOC_STATS appears to be defined, so this is valid.  Modulo TSAN issues with the compiler, the most likely downside would be bad stats, but this may be a TSAN issue.

** CID 1221153:  Concurrent data access violations  (MISSING_LOCK)
/memory/mozjemalloc/jemalloc.c: 4248 in arena_malloc_small()


________________________________________________________________________________________________________
*** CID 1221153:  Concurrent data access violations  (MISSING_LOCK)
/memory/mozjemalloc/jemalloc.c: 4248 in arena_malloc_small()
4242     	if (ret == NULL) {
4243     		malloc_spin_unlock(&arena->lock);
4244     		return (NULL);
4245     	}
4246     
4247     #ifdef MALLOC_STATS
>>>     CID 1221153:  Concurrent data access violations  (MISSING_LOCK)
>>>     Accessing "bin->stats.nrequests" without holding lock "malloc_mutex_s.lock". Elsewhere, "malloc_bin_stats_s.nrequests" is accessed with "malloc_mutex_s.lock" held 7 out of 9 times (2 of these accesses strongly imply that it is necessary).
4248     	bin->stats.nrequests++;
4249     	arena->stats.nmalloc_small++;
4250     	arena->stats.allocated_small += size;
4251     #endif
4252     	malloc_spin_unlock(&arena->lock);
4253

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Also this.  and there are a lot of reports about more minor issues in the jemalloc landing

** CID 1286238:  Concurrent data access violations  (MISSING_LOCK)
/memory/jemalloc/src/src/prof.c: 1079 in prof_tctx_merge_gctx()


________________________________________________________________________________________________________
*** CID 1286238:  Concurrent data access violations  (MISSING_LOCK)
/memory/jemalloc/src/src/prof.c: 1079 in prof_tctx_merge_gctx()
1073     static void
1074     prof_tctx_merge_gctx(tsdn_t *tsdn, prof_tctx_t *tctx, prof_gctx_t *gctx)
1075     {
1076     
1077     	malloc_mutex_assert_owner(tsdn, gctx->lock);
1078     
>>>     CID 1286238:  Concurrent data access violations  (MISSING_LOCK)
>>>     Accessing "gctx->cnt_summed.curobjs" without holding lock "malloc_mutex_s.lock". Elsewhere, "prof_cnt_s.curobjs" is accessed with "malloc_mutex_s.lock" held 3 out of 5 times (1 of these accesses strongly imply that it is necessary).
1079     	gctx->cnt_summed.curobjs += tctx->dump_cnts.curobjs;
1080     	gctx->cnt_summed.curbytes += tctx->dump_cnts.curbytes;
1081     	if (opt_prof_accum) {
1082     		gctx->cnt_summed.accumobjs += tctx->dump_cnts.accumobjs;
1083     		gctx->cnt_summed.accumbytes += tctx->dump_cnts.accumbytes;
1084     	}

Comment 2

[Mike Hommey [:glandium]]

I'm fairly sure the lack of locking for those is deliberate. Jason, can you confirm?

Comment 3

[Jason Evans]

Re: the issue reported in arena_malloc_small(), the lock that provides protection is clearly being unlocked just after (line 4252), so unless there are much bigger problems, this appears to be due to Coverity not fully understanding the locking primitives being used.

Re: the issue reported in prof_tctx_merge_gctx(), gctx->lock protects access.  There are five classes of locks for the heap profiling code, and the report appears to incorrectly infer one of the other classes is responsible for protection here.  It's plausible that there's a path into this code that fails to lock gctx->lock, but if so I can't tell what it is from this report, and the malloc_mutex_assert_owner() call above (line 1077) would fail in a debug build.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Randell Jesup [:jesup] from comment #0)
> Note: MALLOC_STATS appears to be defined, so this is valid.  Modulo TSAN
> issues with the compiler, the most likely downside would be bad stats, but
> this may be a TSAN issue.
> 
> ** CID 1221153:  Concurrent data access violations  (MISSING_LOCK)
> /memory/mozjemalloc/jemalloc.c: 4248 in arena_malloc_small()
> 
> 
> _____________________________________________________________________________
> ___________________________
> *** CID 1221153:  Concurrent data access violations  (MISSING_LOCK)
> /memory/mozjemalloc/jemalloc.c: 4248 in arena_malloc_small()
> 4242     	if (ret == NULL) {
> 4243     		malloc_spin_unlock(&arena->lock);
> 4244     		return (NULL);
> 4245     	}
> 4246     
> 4247     #ifdef MALLOC_STATS
> >>>     CID 1221153:  Concurrent data access violations  (MISSING_LOCK)
> >>>     Accessing "bin->stats.nrequests" without holding lock "malloc_mutex_s.lock". Elsewhere, "malloc_bin_stats_s.nrequests" is accessed with "malloc_mutex_s.lock" held 7 out of 9 times (2 of these accesses strongly imply that it is necessary).
> 4248     	bin->stats.nrequests++;
> 4249     	arena->stats.nmalloc_small++;
> 4250     	arena->stats.allocated_small += size;
> 4251     #endif
> 4252     	malloc_spin_unlock(&arena->lock);
> 4253

The nrequests field is gone as of bug 1379890.

(In reply to Randell Jesup [:jesup] from comment #1)
> Also this.  and there are a lot of reports about more minor issues in the
> jemalloc landing
> 
> ** CID 1286238:  Concurrent data access violations  (MISSING_LOCK)
> /memory/jemalloc/src/src/prof.c: 1079 in prof_tctx_merge_gctx()

Everything under memory/jemalloc is gone as of bug 1363992.