211265 - Log doesn't check the return value of PL_DHashTableInit

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[timeless]

This is a code inspection bug.

QA:
To verify that this bug is fixed, make sure that all callers in of 
PL_DHashTableInit in the indicated class/file check and handle a failure return 
result.
To verify that this bug is invalid, simply verify (e.g. with lxr) that 
PL_DHashTableInit is no longer used by the indicated class/file.

PL_DHashTableInit returns a PRBool indicating whether it succeeded. 
PL_DHashTableInit *can* fail. Code can not assume that table->ops or table-
>data will be null (in fact it probably will not be), although in all 
likelyhood table->entryStore will be null it probably isn't safe to assume this.

Code at time of bug filing:

 159 Log(void* addr)
 160 {
 161 static int initialized = 0;
 162
 163     addr = (void*) ((unsigned) addr - 5);
 164
 165     if (! initialized) {
 166         initialized = 1;
 167         PL_DHashTableInit(&Calls, &Ops, 0, sizeof(CallEntry), 16);
 168     }

Comment 1

[timeless]

Attachment: changes

note that trace.cpp is a windows specific file which doesn't build today :)

at the very least it needs a REQUIRES=xpcom line to pull in pldhash.

Comment 2

[Darin Fisher]

Comment on attachment 126829 [details] [diff] [review]
changes

>Index: mozilla/config/trace.cpp

>+        if (!modInitialized) {
>             modInitialized = 1;
>             PL_DHashTableInit(&Modules, &ModOps, 0, sizeof(ModulesEntry), 16);
>         }

why don't you want to check the return value of PL_DHashTableInit in this case?

Comment 3

[Darin Fisher]

so why bother fixing this code if it isn't used?  should it be cvs removed instead?

Comment 4

[timeless]

Attachment: changes take 2

I'm auditing the code, so that means i want to fix all the broken callers.
i don't really care whether the file is killed or fixed, both will work for me.


that said, this file seems interesting, so i might actually try using it (it
doesn't look very busted). but that's a task for another day.

Comment 5

[hacker formerly known as [e-mail]]

Hmm.  So based upon the comments in the file, this file is used to generate the
win32.order files (I always wondered about those).  But it looks like no one has
actively used MOZ_COVERAGE since we switched to gmake.  If you remove that file,
you should remove the rest of the MOZ_COVERAGE code.

Comment 6

[Darin Fisher]

Comment on attachment 126842 [details] [diff] [review]
changes take 2

r+sr=darin fwiw

i think it'd be good idea to follow up on this to see if this should be
removed.  timeless: can you do that?

Comment 7

[timeless]

checked in