Open
Bug 1162079
Opened 9 years ago
Updated 1 year ago
valgrind/memcheck: Uninitialized value usage in deflate.c (modules/zlib/src/deflate.c)
Categories
(Core :: General, defect)
Tracking
()
NEW
People
(Reporter: ishikawa, Unassigned)
Details
After refreshing C-C source tree about 24 hous ago, I notied a warning from valgrind/memcheck when I test C-C TB under its own |make mozmill| test suite. I have not seen this for the last few weeks when I tried to run |make mozmill| under valgrind/memcheck. So the appearance of this warning is rather new although the relevant source code seems to be old. Two slightly different stacks. (There were several cases.) (1) ==NNNNN== Thread 20 StartupCache: ==NNNNN== Conditional jump or move depends on uninitialised value(s) ==NNNNN== at 0xAB457DB: fill_window (deflate.c:1444) ==NNNNN== by 0xAB46181: deflate_fast (deflate.c:1642) ==NNNNN== by 0xAB47203: MOZ_Z_deflate (deflate.c:903) ==NNNNN== by 0x7D02C00: nsDeflateConverter::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (nsDeflateConverter.cpp:129) ==NNNNN== by 0x7D02F8B: nsZipDataStream::ProcessData(nsIRequest*, nsISupports*, char*, unsigned long, unsigned int) (nsZipDataStream.cpp:147) ==NNNNN== by 0x7D07CE8: nsZipDataStream::ReadStream(nsIInputStream*) (nsZipDataStream.cpp:175) ==NNNNN== by 0x7D0832D: nsZipWriter::AddEntryStream(nsACString_internal const&, long, int, nsIInputStream*, bool, unsigned int) (nsZipWriter.cpp:504) ==NNNNN== by 0x7D0904D: nsZipWriter::AddEntryStream(nsACString_internal const&, long, int, nsIInputStream*, bool) (nsZipWriter.cpp:448) ==NNNNN== by 0xA4B6506: mozilla::scache::CacheCloseHelper(nsACString_internal const&, nsAutoPtr<mozilla::scache::CacheEntry>&, void*) (StartupCache.cpp:416) ==NNNNN== by 0xA4B9329: nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::scache::CacheEntry>, mozilla::scache::CacheEntry*>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (nsBaseHashtable.h:412) ==NNNNN== by 0x72DE407: PL_DHashTableEnumerate(PLDHashTable*, PLDHashOperator (*)(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*), void*) (pldhash.cpp:781) ==NNNNN== by 0xA4B8A48: mozilla::scache::StartupCache::WriteToDisk() (nsBaseHashtable.h:208) ==NNNNN== by 0xA4B8DB5: mozilla::scache::StartupCache::ThreadedWrite(void*) (StartupCache.cpp:540) ==NNNNN== by 0x40C39FC: _pt_root (ptthread.c:212) ==NNNNN== by 0x4A2B0A3: start_thread (pthread_create.c:309) ==NNNNN== by 0x574C04C: clone (clone.S:111) ==NNNNN== Uninitialised value was created by a heap allocation ==NNNNN== at 0x4028BFF: malloc (vg_replace_malloc.c:299) ==NNNNN== by 0xAB5030C: MOZ_Z_zcalloc (zutil.c:310) ==NNNNN== by 0xAB48143: MOZ_Z_deflateInit2_ (deflate.c:294) ==NNNNN== by 0x7D01791: nsDeflateConverter::Init() (nsDeflateConverter.cpp:50) ==NNNNN== by 0x7D027EF: nsDeflateConverter::AsyncConvertData(char const*, char const*, nsIStreamListener*, nsISupports*) (nsDeflateConverter.cpp:95) ==NNNNN== by 0x7D06B74: nsZipDataStream::Init(nsZipWriter*, nsIOutputStream*, nsZipHeader*, int) (nsZipDataStream.cpp:49) ==NNNNN== by 0x7D0830E: nsZipWriter::AddEntryStream(nsACString_internal const&, long, int, nsIInputStream*, bool, unsigned int) (nsZipWriter.cpp:498) ==NNNNN== by 0x7D0904D: nsZipWriter::AddEntryStream(nsACString_internal const&, long, int, nsIInputStream*, bool) (nsZipWriter.cpp:448) ==NNNNN== by 0xA4B6506: mozilla::scache::CacheCloseHelper(nsACString_internal const&, nsAutoPtr<mozilla::scache::CacheEntry>&, void*) (StartupCache.cpp:416) ==NNNNN== by 0xA4B9329: nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::scache::CacheEntry>, mozilla::scache::CacheEntry*>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (nsBaseHashtable.h:412) ==NNNNN== by 0x72DE407: PL_DHashTableEnumerate(PLDHashTable*, PLDHashOperator (*)(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*), void*) (pldhash.cpp:781) ==NNNNN== by 0xA4B8A48: mozilla::scache::StartupCache::WriteToDisk() (nsBaseHashtable.h:208) ==NNNNN== by 0xA4B8DB5: mozilla::scache::StartupCache::ThreadedWrite(void*) (StartupCache.cpp:540) ==NNNNN== by 0x40C39FC: _pt_root (ptthread.c:212) ==NNNNN== by 0x4A2B0A3: start_thread (pthread_create.c:309) ==NNNNN== by 0x574C04C: clone (clone.S:111) ==NNNNN== (2) Usage point has slightly different stack trace. But the data creation stack is the same. ==NNNNN== Thread 30 StartupCache: ==NNNNN== Conditional jump or move depends on uninitialised value(s) ==NNNNN== at 0xAB457DB: fill_window (deflate.c:1444) ==NNNNN== by 0xAB46181: deflate_fast (deflate.c:1642) ==NNNNN== by 0xAB47203: MOZ_Z_deflate (deflate.c:903) ==NNNNN== by 0x7D02D6E: nsDeflateConverter::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (nsDeflateConverter.cpp:165) ==NNNNN== by 0x7D0781D: nsZipDataStream::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (nsZipDataStream.cpp:102) ==NNNNN== by 0x7D07D92: nsZipDataStream::ReadStream(nsIInputStream*) (nsZipDataStream.cpp:184) ==NNNNN== by 0x7D0832D: nsZipWriter::AddEntryStream(nsACString_internal const&, long, int, nsIInputStream*, bool, unsigned int) (nsZipWriter.cpp:504) ==NNNNN== by 0x7D0904D: nsZipWriter::AddEntryStream(nsACString_internal const&, long, int, nsIInputStream*, bool) (nsZipWriter.cpp:448) ==NNNNN== by 0xA4B6506: mozilla::scache::CacheCloseHelper(nsACString_internal const&, nsAutoPtr<mozilla::scache::CacheEntry>&, void*) (StartupCache.cpp:416) ==NNNNN== by 0xA4B9329: nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::scache::CacheEntry>, mozilla::scache::CacheEntry*>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (nsBaseHashtable.h:412) ==NNNNN== by 0x72DE407: PL_DHashTableEnumerate(PLDHashTable*, PLDHashOperator (*)(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*), void*) (pldhash.cpp:781) ==NNNNN== by 0xA4B8A48: mozilla::scache::StartupCache::WriteToDisk() (nsBaseHashtable.h:208) ==NNNNN== by 0xA4B8DB5: mozilla::scache::StartupCache::ThreadedWrite(void*) (StartupCache.cpp:540) ==NNNNN== by 0x40C39FC: _pt_root (ptthread.c:212) ==NNNNN== by 0x4A2B0A3: start_thread (pthread_create.c:309) ==NNNNN== by 0x574C04C: clone (clone.S:111) ==NNNNN== Uninitialised value was created by a heap allocation ==NNNNN== at 0x4028BFF: malloc (vg_replace_malloc.c:299) ==NNNNN== by 0xAB5030C: MOZ_Z_zcalloc (zutil.c:310) ==NNNNN== by 0xAB48143: MOZ_Z_deflateInit2_ (deflate.c:294) ==NNNNN== by 0x7D01791: nsDeflateConverter::Init() (nsDeflateConverter.cpp:50) ==NNNNN== by 0x7D027EF: nsDeflateConverter::AsyncConvertData(char const*, char const*, nsIStreamListener*, nsISupports*) (nsDeflateConverter.cpp:95) ==NNNNN== by 0x7D06B74: nsZipDataStream::Init(nsZipWriter*, nsIOutputStream*, nsZipHeader*, int) (nsZipDataStream.cpp:49) ==NNNNN== by 0x7D0830E: nsZipWriter::AddEntryStream(nsACString_internal const&, long, int, nsIInputStream*, bool, unsigned int) (nsZipWriter.cpp:498) ==NNNNN== by 0x7D0904D: nsZipWriter::AddEntryStream(nsACString_internal const&, long, int, nsIInputStream*, bool) (nsZipWriter.cpp:448) ==NNNNN== by 0xA4B6506: mozilla::scache::CacheCloseHelper(nsACString_internal const&, nsAutoPtr<mozilla::scache::CacheEntry>&, void*) (StartupCache.cpp:416) ==NNNNN== by 0xA4B9329: nsBaseHashtable<nsCStringHashKey, nsAutoPtr<mozilla::scache::CacheEntry>, mozilla::scache::CacheEntry*>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (nsBaseHashtable.h:412) ==NNNNN== by 0x72DE407: PL_DHashTableEnumerate(PLDHashTable*, PLDHashOperator (*)(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*), void*) (pldhash.cpp:781) ==NNNNN== by 0xA4B8A48: mozilla::scache::StartupCache::WriteToDisk() (nsBaseHashtable.h:208) ==NNNNN== by 0xA4B8DB5: mozilla::scache::StartupCache::ThreadedWrite(void*) (StartupCache.cpp:540) ==NNNNN== by 0x40C39FC: _pt_root (ptthread.c:212) ==NNNNN== by 0x4A2B0A3: start_thread (pthread_create.c:309) ==NNNNN== by 0x574C04C: clone (clone.S:111) ==NNNNN== (cf. PID was turned into NNNNN to make comparison easier.) So I look at the usage point and data creation point. http://mxr.mozilla.org/comm-central/source/mozilla/modules/zlib/src/deflate.c Date Usage point: deflate.c: 1444 1440 #ifndef FASTEST 1441 p = &s->prev[n]; 1442 do { 1443 m = *--p; * 1444 *p = (Pos)(m >= wsize ? m-wsize : NIL); 1445 /* If n is not on any hash chain, prev[n] is garbage but 1446 * its value will never be used. 1447 */ 1448 } while (--n); 1449 #endif Data creation point: deflate.c: 294 290 s->hash_mask = s->hash_size - 1; 291 s->hash_shift = ((s->hash_bits+MIN_MATCH-1)/MIN_MATCH); 292 293 s->window = (Bytef *) ZALLOC(strm, s->w_size, 2*sizeof(Byte)); * 294 s->prev = (Posf *) ZALLOC(strm, s->w_size, sizeof(Pos)); 295 s->head = (Posf *) ZALLOC(strm, s->hash_size, sizeof(Pos)); 296 line 294 looks innocent, and so I got curious and here is the line 310 of zutil.c that is eventually called: http://mxr.mozilla.org/comm-central/source/mozilla/modules/zlib/src/zutil.c#310 304 voidpf ZLIB_INTERNAL zcalloc (opaque, items, size) 305 voidpf opaque; 306 unsigned items; 307 unsigned size; 308 { 309 if (opaque) items += size - size; /* make compiler happy */ * 310 return sizeof(uInt) > 2 ? (voidpf)malloc(items * size) : 311 (voidpf)calloc(items, size); 312 } I suspect that |zcalloc| above (which probably is turned into MOZ_Z_calloc by some macro magic or link-time name mutation) can be modified to call |calloc| always. However, I found code looks rather obscure to touch it for now until I hear from people in the know. So I am reporting this error. I am puzzled at what the difference betwen line 310 and 311 other than clearing the content with zero with |calloc()|. And, for that matter, why don't we want to call |calloc()| when |sizeof(uInt)| is larger than 2? Also, line 309 exists solely to shut up "Argument |opaque| is not used" type of error message? I wish there WERE better comments. If the original author(s) thought |calloc| was handy to eliminate this type of uninitialized memory value errors, but too expensive to use it on a larger machine (32 bit vs 16 bit CPU in the early 1990's: the code seems to have evolved from the original version in the 1980's, I think.), I think we can forget about the cost issue today and always call |calloc()| here. At least, this would have avoided the memory uninitialization error. I am not sure if zeroing the memory area is the right solution, but on 16-bit CPU where sizeof(uInt) == 2, I think the code was running properly by calling calloc(), and so we should be safe. Correct? That this suddenly appeared lately in test run of |make mozmill| test suite of C-C thunderbird under valgrind/memcheck suggests maybe a link-time memory layout has changed and exposed this memory error. Or maybe the elimination of some memory alloction and deallocation routines from mozilla source code that happened in the last month or so had played a role by changing memory layout somewhat? Anyway, when I read the code in zcalloc() excerpted above, I deduce that we certainly have a possibility of forgeting to initialize certain memory area before use when sizeof(uInt) > 2 and TB calls malloc() instead of calloc(). TIA
|
Reporter | |
Comment 1•9 years ago
|
||
I forgot the possibility. The change of data |deflation()| needs to handle might have changed to expose this dormant error warning. So this can be data-dependent and I think this really needs to be fixed. TIA
|
|
Reporter | |
Updated•9 years ago
|
Severity: normal → major
|
||
Comment 2•1 year ago
|
||
In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.
Severity: major → --
|
||
Comment 3•1 year ago
|
||
Old bug, but lets at least get it into the correct component
Product: Toolkit → Core
|
||
Updated•1 year ago
|
Severity: -- → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•