Closed Bug 164363 Opened 22 years ago Closed 22 years ago

nsDiskCacheEntry::Size() is incorrect

Categories

(Core :: Networking: Cache, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: bbaetz, Assigned: gordon)

Details

Attachments

(1 file)

(stephend: This isn't the cache UMR you found, but I'm curious if purify can
catch this; valgrind did)

    PRUint32        Size()    { return sizeof(nsDiskCacheEntry)
                                     - sizeof(char) // subtract default key size
                                     + mKeySize     // plus actual key size
                                     + mMetaDataSize;
                              }

is wrong, because sizeof(nsDiskCacheEntry) is 40, because the |char
mKeyStart[1]| causes the struct to have trailing padding. For example, |struct {
int a, char b }| has a sizeof 8, in both c and c++, with gcc 2.96 on linux.

When you then subtract sizeof(char), you end up with Size() returning three
bytes too many, so the memset for the padding in CreateDiskCacheEntry misses the
last three bytes.

When PR_Write writes the block out, valgrind notices that only 509 of the 512
specified bytes have been initialised, and it warns:

==8730== Syscall param write(buf) contains uninitialised or unaddressable byte(s)
==8730==    at 0x420DAE24: (within /lib/i686/libc-2.2.5.so)
==8730==    by 0x4043EB74: pt_Write
(/home/bbaetz/src/mozilla/nsprpub/pr/src/pthreads/ptio.c:1295)
==8730==    by 0x40427E71: PR_Write
(/home/bbaetz/src/mozilla/nsprpub/pr/src/io/priometh.c:142)
==8730==    by 0x45A59A15: nsDiskCacheBlockFile::WriteBlocks(void *, int, int)
(/home/bbaetz/src/mozilla/netwerk/cache/src/nsDiskCacheBlockFile.cpp:287)
==8730==    Address 0x441AC9A5 is 509 bytes inside a block of size 512 alloc'd
==8730==    at 0x400429EC: __builtin_vec_new (vg_clientfuncs.c:152)
==8730==    by 0x45A5D503: CreateDiskCacheEntry(nsDiskCacheBinding *)
(/home/bbaetz/src/mozilla/netwerk/cache/src/nsDiskCacheEntry.cpp:113)
==8730==    by 0x45A5EF20:
nsDiskCacheMap::WriteDiskCacheEntry(nsDiskCacheBinding *)
(/home/bbaetz/src/mozilla/netwerk/cache/src/nsDiskCacheMap.cpp:641)
==8730==    by 0x45A5A9FB: nsDiskCacheDevice::DeactivateEntry(nsCacheEntry *)
(/home/bbaetz/src/mozilla/netwerk/cache/src/nsDiskCacheDevice.cpp:513)

Subtracting 4 probably isn't a good idea, you probably want to hardcode in
9*sizeof(PRUint32) and assume that there is no padding between ints, or use
offsetof on the char variable. Not sure if thats implemented everywhere, though;
you may need to define your own.

As well, since you do seem to be playing with allocating to char* and then
casting to the nsDiskCacheEntry POD type, a comment mentioning that adding
constructors/destructor/virtual methods will break stuff is probably a good idea.
nsAsyncStreamListener.cpp uses offsetof(), so it should be safe for us to use it
here.  I'll work up a patch.
Status: NEW → ASSIGNED
nsAsyncStreamListener.cpp is actually wrong, because its undefined to use
offsetof on a class with constructors/destructors/virtual methods. :) Newer
g++'s warn about this.

That shouldn't be a problem here, though, since nsDiskCacheEntry is a
POD-struct, and so that restriction doesn't apply.
Comment on attachment 102292 [details] [diff] [review]
Size() rewritten using offsetof()

r=sdagley
Attachment #102292 - Flags: review+
Comment on attachment 102292 [details] [diff] [review]
Size() rewritten using offsetof()

yup, this looks good. sr=darin
Attachment #102292 - Flags: review+ → superreview+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
verified - checked in on trunk
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: