leaks, double frees in DBM's __hash_open() error paths

RESOLVED FIXED in 3.10

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

NSS often calls DBM's __hash_open() to try to open files that typically do not
exist (e.g. cert5.db), or that are otherwise corrupt or invalid.  

Each time __hash_open() fails, it 
a) leaks the strdup'ed file name string saved in hashp->filename, and/or
b) if it fails in alloc_segs(), it will double-free the hashp struct, and
its allocated members.  

When the function alloc_segs fails, it calls hdestroy, which frees all the 
components of hashp and free's hashp itself.  But when it returns, the 
caller still thinks hashp is valid, and may attempt to free it again, and/or
attempt to access the freed structure.

The solution to these problems is to ensure that hdestroy gets called 
exactly once in all error paths.  I will attach a patch that does just that.
(Assignee)

Comment 1

15 years ago
Nominating for NSS 3.9
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.9
(Assignee)

Comment 2

15 years ago
Created attachment 138456 [details] [diff] [review]
patch -  v1

This patch:
a) eliminates lots of	 #if 0	 code.
b) removes calls to hdestroy from static functions called by __hash_open()
c) changes __hash_open to call hdestroy(), rather than free(), in the error
exit path, thereby assuring that all allocated components of hashp get 
freed once.
d) corrects some indentation problems.
(Assignee)

Comment 3

15 years ago
Comment on attachment 138456 [details] [diff] [review]
patch -  v1

This patch also eliminates some bogus code that destroys the DB file if it
exhibits the results of a certain error that was fixed back before 1996.
Attachment #138456 - Flags: review?(wchang0222)

Comment 4

15 years ago
Comment on attachment 138456 [details] [diff] [review]
patch -  v1

I will review this patch today.  Bob, could you review it, too?

It seems that we don't need to fix this bug in NSS 3.9 because
the memory leaks only occur during NSS initialization and the
double free only occurs if calloc() fails.
Attachment #138456 - Flags: superreview?(rrelyea0264)
(Assignee)

Comment 5

15 years ago
Consider the case of switching profiles after the client (e.g. browser) has
been running for a while.  calloc could fail while in NSS_Init.

Comment 6

15 years ago
Comment on attachment 138456 [details] [diff] [review]
patch -  v1

This patch is basically good except for some minor
problems.

1. We cannot assume that malloc and calloc set errno
to ENOMEM on failure because Standard C does not
require these functions to set errno when they fail.
So it is better to set errno to ENOMEM explicitly
after a malloc or calloc call fails.

2. Some of the changes are not necessary.  In particular
changing "return (NULL);" to "goto fail;" and
"fail: return NULL;"

3. I don't know if it's okay to remove the close(hashp->fp)
and remove(file) calls.  Also the original code sets errno
to ENOENT but the new code sets errno to ETYPE.

>-		if (hashp->NKEYS < 0) {
>-		    /*
>-		    ** OOPS. Old bad database from previously busted
>-		    ** code. Blow it away.
>-		    */
>-		    close(hashp->fp);
>-		    if (remove(file) < 0) {
...
>-		    } else {
...
>-		    }
>-		    RETURN_ERROR(ENOENT, error0);
>-		}
>+		if (hashp->NKEYS < 0) /* OOPS. Old bad database */
>+			RETURN_ERROR(EFTYPE, error1);
(Assignee)

Comment 7

15 years ago
Regarding comment 6:
1. Useful to know that about malloc and calloc.  I think that alone is 
sufficient to require another revision of this patch.

2. True.  This change was a side effect of having tried several approaches
to fixing the double-free problem.  It's not necessary.

3. The negative NKEYS bug was an ANCIENT bug, introduced and fixed in Navigator
2.x or 3.x, over 7 years ago, before I started here.  There are certainly no DBs
now in use that have that problem.

I consider blowing away the customer's files to be evil.  Reporting ENOENT 
was an attempt to cover up that act.  After blowing away the customer's file,
the author wanted to make it appear that the file had not been there at all,
rather than appearing that it had been there and the code had removed it. 

Since the file is no longer being removed, it is in appropriate to return 
that error code.  The ETYPE error is consistent with all the other errors
returned in this function for corrupt files.

This bug is severity=normal only because I didn't set it to anything else.
I haven't seen this failure, only found it in code review.  Yet a potential
double-free does seem serious enough to warrant fixing it now.  However,
I can live with this bug being fixed after NSS 3.9.

Comment 8

15 years ago
Nelson, thank you for explaining the "if (hashp->NKEYS < 0)"
change that removes a bad workaround for an ancient bug.

Comment 9

15 years ago
patch looks good to me. Since you are making changes anyway, though:

it appears that error1: and error0: no longer need to be separate labels and can
be consolidated.

BTW when checking out the original source, it appears that I have already
applied this patch to my local tree, nelson did you release this patch sometime
in the past for testing?

bob
(Assignee)

Comment 10

15 years ago
I previously emailed a larger patch that included numerous assertions and
other things to NSS team members.  This patch is a subset of that patch.
Perhaps you should start with a fresh DBM source and apply this patch to it.
But I will revise this patch, in light of Wan-Teh's comments above. 
However, I am no longer targeting this for NSS 3.9.
Target Milestone: 3.9 → 3.9.1

Comment 11

15 years ago
Comment on attachment 138456 [details] [diff] [review]
patch -  v1

see comments in bug
Attachment #138456 - Flags: superreview?(rrelyea0264) → superreview+
(Assignee)

Comment 12

15 years ago
Comment on attachment 138456 [details] [diff] [review]
patch -  v1

Marking this obsolete, in light of Wan-Teh's observations about ENOMEM.
New patch forthcoming.	
Wan-Teh, please mark this patch r-, thanks.
Attachment #138456 - Attachment is obsolete: true

Updated

15 years ago
Attachment #138456 - Flags: review?(wchang0222) → review-
(Assignee)

Comment 13

15 years ago
Created attachment 140049 [details] [diff] [review]
patch v2

hash.c was meant to be viewed with tabstops set to 4 spaces, 
so I have run this patch through pr -e4 to make it appear as it would 
in an editor set that way.  
I can attach a copy of the unaltered patch if desired.
(Assignee)

Comment 14

15 years ago
Comment on attachment 140049 [details] [diff] [review]
patch v2

please review.

This patch also incorporates Bob's prior feedback, suggesting that the labels
error0 and error1 be collapsed into a single label.
Attachment #140049 - Flags: review?(wchang0222)
(Assignee)

Comment 15

15 years ago
patch v2 is a patch to the DBM_1_6_BRANCH.  
But I wonder.  
Now that NSS is the sole remaining user of DBM in mozilla, maybe we should 
"whomp" the current branch onto the trunk, and release DBM_1_7 from the trunk.
Agreed?

Comment 16

15 years ago
Comment on attachment 140049 [details] [diff] [review]
patch v2

r=wtc.

I haven't fully reviewed all the implications of calling
hdestroy on a hash table that can be in various stages of
being constructed.  This patch does not change hdestroy,
but calls hdestroy under more conditions.  It is not easy
to verify that hdestroy can properly destroy any partially
constructed hash table.

For example, we may pass a hash table with hashp->fp = -1
to hdestroy, which in turn passes the hash table to
flush_meta.  flush_meta passes hashp->fp to lseek and
write without checking.  lseek will fail with errno = EBADF,
causing flush_meta to fail.  This causes hdestroy to fail.
Since the patch ignores the return value of hdestroy,
it'll work out fine, but this is just an example of the
implications I haven't fully reviewed.
Attachment #140049 - Flags: superreview?(rrelyea0264)
Attachment #140049 - Flags: review?(wchang0222)
Attachment #140049 - Flags: review+

Comment 17

15 years ago
Comment on attachment 140049 [details] [diff] [review]
patch v2

I just realized that the scenario I just described can
never happen because if hashp->fp is -1, hashp->save_file
is 0, which will cause flush_meta to return 0 immediately.
Still, it is an example of a code path that a full review
of this patch must go through.

Comment 18

15 years ago
Re: comment 15

Yes, we can release DBM 1.7 from the trunk.
(Assignee)

Comment 19

15 years ago
Checked in on the DBM_1_6_BRANCH.

/cvsroot/mozilla/dbm/src/hash.c,v  <--  hash.c
new revision: 3.15.2.5; previous revision: 3.15.2.4

We need to generate a DBM 1.7 for NSS 3.9.1 
(if this bug is to remain targetted at NSS 3.9.1).  

I will not mark this fixed until we have a plan for DBM 1.7
(Assignee)

Updated

15 years ago
Blocks: 232483
(Assignee)

Comment 20

15 years ago
Marking fixed.  Bug 232483 has been created to track the creation of a new
DBM release.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 21

15 years ago
Comment on attachment 140049 [details] [diff] [review]
patch v2

r=relyea
Attachment #140049 - Flags: superreview?(rrelyea0264) → superreview+

Comment 22

14 years ago
I'm reopening this bug, because current releases of NSS still do not contain the
fix, even though it was checked in almost a year ago . The problem is that no
new release of DBM was made which include this fix. A new release of DBM needs
to be made, probably 1.62 .

While looking at CVS logs with Nelson, we found that a number of checkins were
made on the trunk, and a number of other checkins on the DBM_1_6_BRANCH . We
need to sort this out and figure out which source is correct to use, and
possibly merge some patches, before the next release .
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 23

14 years ago
I vote for pulling and building DBM with NSS, and not having DBM releases
that are separate from NSS releases.  I propose that we take the sources
from the DBM_1_6_BRANCH and go forward with them, because not all the 
checkins on the trunk have been reviewed by the NSS team (IINM), and 
there are issues with some of the trunk checkins, IMO.

Comment 24

14 years ago
If we do that, we must create a copy of the
DBM sources inside mozilla/security/dbm.

When Mozilla builds NSS, it uses the trunk
of mozilla/dbm.  So when NSS is built as
part of Mozilla, NSS needs to use the same
cvs tag on dbm as Mozilla.
(Assignee)

Comment 25

14 years ago
I have been informed that mozilla has no use for DBM now, except in NSS.
So, I see no reaosn why mozilla cannot pull DBM with the same tag by 
which they pull NSS.  Do you?

Updated

14 years ago
Target Milestone: 3.9.1 → 3.10

Comment 26

14 years ago
Created attachment 177051 [details] [diff] [review]
Patch for the trunk

I just checked in this patch on the DBM trunk
(Mozilla 1.8 Beta 2).

Comment 27

14 years ago
Now the trunk of mozilla/dbm is ready for our
use.  The only essential differences between the
trunk and DBM_1_6_BRANCH are OS/2 changes.
Those changes literally only affect Julien.

We can mark this bug fixed after the NSS trunk
nightly build at Sun has started to pull the tip
of mozilla/dbm (instead of DBM_1_6_BRANCH).

Comment 28

14 years ago
Created attachment 177053 [details] [diff] [review]
Essential diffs between mozilla/dbm trunk and DBM_1_6_BRANCH

These are the essential changes between the mozilla/dbm
trunk and DBM_1_6_BRANCH.  You can see that they are all
OS/2 changes.  (Please ignore the comment
    /* We can't use fcntl because of NFS bugs. SIGH */
in the diffs.  I found that comment should be deleted,
so I did that.)

Comment 29

14 years ago
Wan-Teh,

You can ignore the OS/2 changes . If something breaks at home, I will open bugs
later . I think most of my OS/2 changes were done too hastily anyway and there
were cleaner fixes possible .
(Assignee)

Comment 30

14 years ago
I'm marking this bug fixed again.  The fix will be in DBM in NSS 3.10
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.