nsDiskCacheEntry::Size() is incorrect

VERIFIED FIXED

Status

()

Core
Networking: Cache
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: bbaetz, Assigned: gordon)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
(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.
(Assignee)

Comment 1

16 years ago
nsAsyncStreamListener.cpp uses offsetof(), so it should be safe for us to use it
here.  I'll work up a patch.
Status: NEW → ASSIGNED
(Reporter)

Comment 2

16 years ago
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.
(Assignee)

Comment 3

16 years ago
Created attachment 102292 [details] [diff] [review]
Size() rewritten using offsetof()

Comment 4

16 years ago
Comment on attachment 102292 [details] [diff] [review]
Size() rewritten using offsetof()

r=sdagley
Attachment #102292 - Flags: review+

Comment 5

16 years ago
Comment on attachment 102292 [details] [diff] [review]
Size() rewritten using offsetof()

yup, this looks good. sr=darin
Attachment #102292 - Flags: review+ → superreview+
(Assignee)

Comment 6

16 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 7

16 years ago
verified - checked in on trunk
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.