After bug 389575 was checked in, I ran into the following error on my debug build: # gmake[6]: Entering directory `/mnt/mozilla/build/seamonkey-dbg/netwerk/cookie/src' # nsCookie.cpp # c++ -o nsCookie.o -c -I../../../dist/include/system_wrappers -include /mnt/mozilla/src/mozilla/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -DOSTYPE=\"Linux2.6.22\" -DOSARCH=Linux -DIMPL_NS_NET -I../../../dist/include/xpcom -I../../../dist/include/string -I../../../dist/include/pref -I../../../dist/include/storage -I../../../dist/include -I../../../dist/include/necko -I../../../dist/include/nspr -DMOZ_PNG_READ -DMOZ_PNG_WRITE -I../../../dist/sdk/include -fPIC -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -fshort-wchar -pthread -pipe -DDEBUG -D_DEBUG -DDEBUG_robert -DTRACING -g -fno-inline -Os -freorder-blocks -fno-reorder-functions -gstabs+ -march=pentium4 -mtune=nocona -msse2 -msse3 -mssse3 -mfpmath=sse -Werror -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/nsCookie.pp /mnt/mozilla/src/mozilla/netwerk/cookie/src/nsCookie.cpp # cc1plus: warnings being treated as errors # /mnt/mozilla/src/mozilla/netwerk/cookie/src/nsCookie.cpp: In member function 'virtual nsrefcnt nsCookie::AddRef()': # /mnt/mozilla/src/mozilla/netwerk/cookie/src/nsCookie.cpp:158: warning: comparison is always true due to limited range of data type # gmake[6]: *** [nsCookie.o] Error 1 # gmake[6]: Leaving directory `/mnt/mozilla/build/seamonkey-dbg/netwerk/cookie/src' After going the painful way of reducing a nsCookie.i file to a simple testcase, that's what I ended up with: --------------------------------- typedef unsigned int PRUint32; typedef int PRInt32; typedef PRUint32 nsrefcnt; class nsCookie { public: virtual __attribute__ ((visibility ("hidden"))) nsrefcnt __attribute__ ((regparm (0), cdecl)) AddRef(void); protected: PRUint32 mRefCnt : 16; }; nsrefcnt nsCookie::AddRef(void) { do { if (!(PRInt32(mRefCnt) >= 0)) { } } while (0); ++mRefCnt; return mRefCnt; } --------------------------------- Ouput there is: > c++ -o nsCookieTest.o -Wall -Werror nsCookie.i cc1plus: warnings being treated as errors nsCookie.i: In member function ‘virtual nsrefcnt nsCookie::AddRef()’: nsCookie.i:16: warning: comparison is always true due to limited range of data type --------------------------------- Blame goes to http://mxr.mozilla.org/mozilla/source/netwerk/cookie/src/nsCookie.cpp#158 calling http://mxr.mozilla.org/mozilla/source/xpcom/glue/nsISupportsImpl.h#273 with the mRefCnt being defined in http://mxr.mozilla.org/mozilla/source/netwerk/cookie/src/nsCookie.h#154

bug 200632 made this a bitfield

this splits the bitfield up and should fix the problem. sizeof(nsCookie) remained at 80 bytes before and after patch (on linux x86_64), so there doesn't appear to be any detrimental effect due to alignment or anything.

Using only 16 bits for a reference count sounds pretty sketchy. Does this code somehow know that there will never be more than 60000 references to a nsCookie object? Why PRInt16 instead of PRUInt16?

(In reply to comment #2) > this splits the bitfield up and should fix the problem. sizeof(nsCookie) > remained at 80 bytes before and after patch (on linux x86_64), so there doesn't > appear to be any detrimental effect due to alignment or anything. Check on Windows with VC++; I doubt this holds there: http://www.mozilla.org/hacking/portable-cpp.html#same_type_bitfields

Er, I'm probably wrong, because the bug that motivated that addition said things were okay with the same type (ignoring the type's signedness), but do still check there to be safe.

good thinking. i don't build on windows; can someone who does please test? ;)

(In reply to comment #3) > Using only 16 bits for a reference count sounds pretty sketchy. Does this code > somehow know that there will never be more than 60000 references to a nsCookie > object? > > Why PRInt16 instead of PRUInt16? unless code is deliberately trying to overflow the refcount, i doubt we'll ever hit that limit, and if so then the same applies to a 32bit refcount as well. do you have another scenario in mind? also, making it PRInt16 is the point of the bug - refcounts are apparently signed.

> unless code is deliberately trying to overflow the refcount, i doubt we'll ever > hit that limit Or an attacker is trying to overflow the refcount by causing code to reference it... > if so then the same applies to a 32bit refcount as well In a 32-bit app, there can't be 2^32 objects referencing a given object, so 32-bit refcounts are safe assuming there aren't any bugs. > making it PRInt16 is the point of the bug - refcounts are apparently signed. Are you sure? From looking at nsISupportsImpl, I got the impression that most refcounts are unsigned 32-bit ints. The compiler warning here might be triggered by the ": 16" rather than the "unsigned" in the type.

(In reply to comment #8) > The compiler warning here might be triggered by the ": 16" rather than the > "unsigned" in the type. I don't think there's any doubt it's anything other than this; an unsigned 32-bit integer with the upper bit set can be cast to a negative, signed 32-bit integer. The same can't hold for a 16-bit integer because it must always fit within a 31-bit unsigned integer and thus never wrap around. > In a 32-bit app, there can't be 2^32 objects referencing a given object, so > 32-bit refcounts are safe assuming there aren't any bugs. Actually, that's a good point -- 32-bit refcounts are safe because there can't be that many things referring to an object. Furthermore, 30-bit refcounts are safe on 32-bit systems because pointers each occupy 32 bits or 4 bytes of memory, so only 2^30 objects could hold references at once. (This trick's used by the cycle collector to store extra information in the refcount it uses.) If you really wanted to do it, you could extend this to a 29-bit refcount under the assumption that any object is going to contain at least a word more state than just a refcount. This is theoretically unsound, but practically it's not a problem for three reasons: first, an empty object with only a refcount is useless, second, nobody's likely to refcount an object containing only a value smaller than 32 bits (and can such an object legally have a sizeof != 8?), and third, I very much doubt any of the compilers we care about allocate with anything other than 32-bit alignment. Beyond that, tho, I think you hit the safe-with-reasonable-assumptions barrier to reducing refcount size any more, and if, say, SameReferrerOnly or similar were added, you'd need to increase the size of nsCookie. For the immediate future you don't need to increase nsCookie's size, but any more tweaks and you'd have to put the bit elsewhere (any of the other pointers might work if you can guarantee they're always word-aligned, but keep in mind bitwise accesses are slow).

re comments 8 and 9, good points. :p Jesse, about an attacker, do you think it'd be a problem? i.e. if an attacker has managed to get you to install their extension (or found some other way to execute such code), are there not better ways to attack than by making you leak to death via refcount? also, Neil has tested this on his windows plat and it aligns favorably.

(In reply to comment #10) > Jesse, about an attacker, do you think it'd be a problem? i.e. if an attacker > has managed to get you to install their extension (or found some other way to > execute such code), are there not better ways to attack than by making you leak > to death via refcount? It's not that; if you wrap around to 0, Release will delete the nsCookie, and all references to it will be stale, and any code that touches the cookie will be using freed memory with possibly-arbitrary contents -- and that's a recipe for disaster. Also, I'm not convinced it's impossible to accumulate an arbitrary number of refcounts without installing an extension.

This patch cascade unwraps NS_IMPL_ISUPPORTS2 and NS_IMPL_ADDREF, and removes NS_PRECONDITION which causes the warning to appear. Since this patch does NOT change anything in the behavior of non-debug applications, it may be committed to fix debug bustage without closing this bug. When there is consensus on the way of fixing this issue, it can be backed out.

So... why is it worth optimizing the size of an nsCookie object? Are there really enough of them around to worry about a 2 byte size increase per object?

Comment on attachment 279422 [details] [diff] [review] v1 i think i agree with biesi and jesse here. the difference under typical usage on a 32-bit system will be 4kb which just isn't worth the trouble of bitfields etc.

use 32-bit refcount from NS_DECL_ISUPPORTS instead.

fixed on trunk.