Log doesn't check the return value of PL_DHashTableInit

RESOLVED FIXED

Status

SeaMonkey
Build Config
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

Trunk
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

2.46 KB, patch
Darin Fisher
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
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     }
(Assignee)

Comment 1

15 years ago
Created attachment 126829 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #126829 - Flags: superreview?(darin)
Attachment #126829 - Flags: review?(darin)

Comment 2

15 years ago
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

15 years ago
so why bother fixing this code if it isn't used?  should it be cvs removed instead?
(Assignee)

Updated

15 years ago
Attachment #126829 - Flags: superreview?(darin)
Attachment #126829 - Flags: review?(darin)
(Assignee)

Comment 4

15 years ago
Created attachment 126842 [details] [diff] [review]
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.
Attachment #126829 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #126842 - Flags: superreview?(darin)
Attachment #126842 - Flags: review?(darin)
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

15 years ago
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?
Attachment #126842 - Flags: superreview?(darin)
Attachment #126842 - Flags: superreview+
Attachment #126842 - Flags: review?(darin)
Attachment #126842 - Flags: review+
(Assignee)

Comment 7

15 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.