All users were logged out of Bugzilla on October 13th, 2018

valgrind/memcheck: Uninitialized value usage in deflate.c (modules/zlib/src/deflate.c)

NEW
Unassigned

Status

()

--
major
4 years ago
3 years ago

People

(Reporter: ishikawa, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
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

4 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

4 years ago
Severity: normal → major
You need to log in before you can comment on or make changes to this bug.