334180 - pldhash/jsdhash should assert that ops don't mutate the table

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[David Baron :dbaron:]

pldhash should assert that its ops don't recursively mutate the table by checking that table->entryCount, table->removedCount, and (maybe) table->generation do not change across calls to ops.  See bug 334177 for a crash that this might have caught sooner.

I still haven't thought of a good way to do this without making the code messy, but I'll try to think about it.  I expect I'll introduce a macro for calling ops, though.

Comment 1

[David Baron :dbaron:]

Then again, much more useful to assert on the inner mutation by adding a DEBUG-only boolean member to PLDHashTable.

Comment 2

[David Baron :dbaron:]

Does the JSDHashTable struct attempt to preserve binary compatibility in a way that would prevent adding a DEBUG_only member?

(Also, bug 313309 comment 20 is a pain.)

Comment 3

[Brendan Eich [:brendan]]

Can someone please resync js and pl here, by hacking on js/src/plify_jsdhash.sed?  That is the rule, please follow it per the generated comment in pldhash.*.

DEBUG-only binary incompatibility is ok by me.

/be

Comment 4

[David Baron :dbaron:]

Attachment: patch

I'll manually re-merge bsmedberg's changes when landing, if that's still needed.

Comment 5

[Benjamin Smedberg]

Attachment: backport pldhash -> jsdhash of win32 fastcall support, rev. 1 (checked in)

Comment 6

[Benjamin Smedberg]

I'm not so happy with DEBUG-only binary incompatibility: we include pldhash in the XPCOM glue static lib, and it's perfectly reasonable for DEBUG client code to link against non-DEBUG xpcomglue_s

Comment 7

[David Baron :dbaron:]

(In reply to comment #6)
> I'm not so happy with DEBUG-only binary incompatibility: we include pldhash in
> the XPCOM glue static lib, and it's perfectly reasonable for DEBUG client code
> to link against non-DEBUG xpcomglue_s

That's actually not a problem in this case since it's at the end of the struct; the only problems would be if:
 (1) client code tried to poke at the member variable explicitly, #ifdef DEBUG, along with a non-debug libxpcom.so
 (2) non-DEBUG component using the struct rather than PL_NewDHashTable along with a DEBUG libxpcom.so

I think (2) is actually a real problem, though.

Comment 8

[David Baron :dbaron:]

Comment on attachment 218710 [details] [diff] [review]
patch

I'll do the less-useful assertions that give the assertion on the way out.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #6)
> I'm not so happy with DEBUG-only binary incompatibility: we include pldhash in
> the XPCOM glue static lib, and it's perfectly reasonable for DEBUG client code
> to link against non-DEBUG xpcomglue_s

FWIW, I've been bitten by mixing DEBUG and non-DEBUG libs and it's a real pain to figure out what went wrong. For me, at least, crashes began happening because of stack corruption and it was difficult to find exactly what was causing it.

I realize that's not exactly a strong argument against the DEBUG-only variable, but if it happened to me then it will probably happen to others, especially once people begin using the gecko SDK more frequently.

Comment 10

[Benjamin Smedberg]

I'm personally not worried about 2), since we're talking about static linkage: it doesn't affect the SDK at all, since that would not be compiled with DEBUG in any released configuration.

Comment 11

[David Baron :dbaron:]

I can just allocate an extra word in the entry store and cover it up with macros.

Comment 12

[David Baron :dbaron:]

Attachment: patch

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 218790 [details] [diff] [review]
patch

>+/*
>+ * The following DEBUG-only code is used to assert that calls to one of
>+ * table->ops or to an enumerator do not cause re-entry into a call that
>+ * can mutate the table.  The recursion level is stored in additional
>+ * space allocated at the end of the entry store to avoid changing
>+ * issues when mixing DEBUG and non-DEBUG components.
>+ */
>+#ifdef DEBUG
>+
>+#define RECURSION_LEVEL(table_) (*(uint32*)(table_->entryStore + \
>+  JS_DHASH_TABLE_SIZE(table_) * table_->entrySize))
>+
>+#define ENTRY_STORE_EXTRA sizeof(uint32)
>+#define INCREMENT_RECURSION_LEVEL(table_) (++RECURSION_LEVEL(table))
>+#define DECREMENT_RECURSION_LEVEL(table_) (--RECURSION_LEVEL(table))

Thanks, only nit is to format the above (and the #else counterparts) like so:

#define RECURSION_LEVEL(table_) (*(uint32*)(table_->entryStore +              \
                                            JS_DHASH_TABLE_SIZE(table_) *     \
                                            table_->entrySize))

#define ENTRY_STORE_EXTRA                   sizeof(uint32)
#define INCREMENT_RECURSION_LEVEL(table_)   (++RECURSION_LEVEL(table))
#define DECREMENT_RECURSION_LEVEL(table_)   (--RECURSION_LEVEL(table))

/be

Comment 14

[David Baron :dbaron:]

And fix the s/table/table_/ which I notice is needed :-)

Comment 15

[David Baron :dbaron:]

Checked in to trunk.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 218790 [details] [diff] [review]
patch

a=me for 1.8 branch, with the nits and macro formal name fixed.

/be

Comment 17

[David Baron :dbaron:]

Attachment: final trunk patch

This is what I landed on the trunk.

Comment 18

[David Baron :dbaron:]

Checked in to MOZILLA_1_8_BRANCH.

Comment 19

[Andreas]

This causes new warnings in optimized builds:

gcc -o Linux_All_OPT.OBJ/jsdhash.o -c -Wall -Wno-format -O -DXP_UNIX -DSVR4 -DSYSV -D_BSD_SOURCE -DPOSIX_SOURCE -DHAVE_LOCALTIME_R -DX86_LINUX  -UDEBUG -DNDEBUG -UDEBUG_ -DEDITLINE -ILinux_All_OPT.OBJ  jsdhash.c
jsdhash.c: In function `JS_DHashTableFinish':
jsdhash.c:364: warning: statement with no effect
jsdhash.c:382: warning: statement with no effect
jsdhash.c: In function `JS_DHashTableOperate':
jsdhash.c:537: warning: statement with no effect
jsdhash.c:631: warning: statement with no effect
jsdhash.c: In function `JS_DHashTableEnumerate':
jsdhash.c:663: warning: statement with no effect
jsdhash.c:711: warning: statement with no effect

Comment 20

[David Baron :dbaron:]

Attachment: make increment and decrement be statements rather than expressions

Yeah, I meant to switch back to this; I was using them as expressions at one point.

Comment 21

[David Baron :dbaron:]

Attachment: make increment and decrement be statements rather than expressions

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 219034 [details] [diff] [review]
make increment and decrement be statements rather than expressions

It's simpler just to (void) cast the real macro defs, and make the stub-macros expand to /* nothing */ rather than (void)0.

/be

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 219034 [details] [diff] [review]
make increment and decrement be statements rather than expressions

Sorry I didn't mention this, I always (void) cast if possible to make expression statements not invite warnings, else use JS_BEGIN_MACRO/JS_END_MACRO.  See jslock.h for an example.

/be

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 218712 [details] [diff] [review]
backport pldhash -> jsdhash of win32 fastcall support, rev. 1 (checked in)

mozilla/js/src/jsdhash.h 	1.33

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 218863 [details] [diff] [review]
final trunk patch

> PLDHashEntryHdr * PL_DHASH_FASTCALL
> PL_DHashTableOperate(PLDHashTable *table, const void *key, PLDHashOperator op)
> {
 ...
>+    NS_ASSERTION(op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0,
>+                 "op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0");
>+    INCREMENT_RECURSION_LEVEL(table);
>+

I can trigger this assertion consistently in a fresh 1.8 branch firefox build during XPCOM shutdown after selecting a new profile from the profile manager. Should this be a new bug?

Comment 26

[David Baron :dbaron:]

(In reply to comment #25)
> I can trigger this assertion consistently in a fresh 1.8 branch firefox build
> during XPCOM shutdown after selecting a new profile from the profile manager.
> Should this be a new bug?

If it's not the same as bug 334605, yes.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #26)
> If it's not the same as bug 334605, yes.

It was :)

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 219034 [details] [diff] [review]
make increment and decrement be statements rather than expressions

Fixed otherwise ((void)[01] expressions).

/be