99248 - use of evil strtok function

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Ulrich Drepper]

xpcom/base/nsTraceMalloc.c is strtok which is not a thread-safe function.  I'll
shortly attach a patch which fixes the problem in a way used elsewhere in the
project.  The change to configure.in is also submitted as part of some other bug
reports so applying it might result in conflicts.

Comment 1

[Ulrich Drepper]

Attachment: avoid using strtok

Comment 2

[Doug Turner (:dougt)]

chris, do the makefile changes look okay?  I am happy with the rest of the patch.

Comment 3

[cls]

yep. r=cls

Comment 4

[rpotts (gone)]

Comment on attachment 48998 [details] [diff] [review]
avoid using strtok

sr=[e-mail]

Comment 5

[Doug Turner (:dougt)]

Thanks for the bug fix.  I have checked it into the trunk:

Checking in nsTraceMalloc.c;
/cvsroot/mozilla/xpcom/base/nsTraceMalloc.c,v  <--  nsTraceMalloc.c
new revision: 1.25; previous revision: 1.24
done

Appartently, the patch to configure.in is already there.

Comment 6

[Mike Shaver (:shaver emeritus)]

So either the code is re-entrant and needs strtok_r -- in which case we need to
do better than just falling back to "evil strtok" if it's not present -- or it's
not re-entrant and the original code was just fine.  (Not to mention clearer.)

Happily, it's the latter: the code in question is called in the unthreaded child
process spawned under --trace-malloc operation.  There are no reentrance or
threadsafety issues.  We should undo the patch, methinks.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did anyone compile this patch before checking it in?  nsnull isn't defined in
nsTraceMalloc.c, since it doesn't bring in nscore.h.  I'm going to change the
use of nsnull to NULL, since that's consistent with the rest of the file.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, what was checked in wasn't the original reviewed patch, but added an
|= nsnull| to the definition of |char *newstr|.

Comment 9

[Doug Turner (:dougt)]

shaver, I see what you are saying - david, how about reverting this patch all
together?

Comment 10

[Brendan Eich [:brendan]]

As Nancy Kerrigan said: "Whyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy?"

Please revert.  Don't fix what ain't broke (I'm sorry I skimmed bugmail this week).

/be

Comment 11

[Doug Turner (:dougt)]

Checking in nsTraceMalloc.c;
/cvsroot/mozilla/xpcom/base/nsTraceMalloc.c,v  <--  nsTraceMalloc.c
new revision: 1.27; previous revision: 1.26
done

Sorry about that.  Don't know what I was thinking.  I have reverted the blunder.