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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Nominating for NSS 3.9

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 4

[Wan-Teh Chang]

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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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);

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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

Comment 9

[Robert Relyea]

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

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 11

[Robert Relyea]

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

see comments in bug

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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.

Comment 17

[Wan-Teh Chang]

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

[Wan-Teh Chang]

Re: comment 15

Yes, we can release DBM 1.7 from the trunk.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Marking fixed.  Bug 232483 has been created to track the creation of a new
DBM release.

Comment 21

[Robert Relyea]

Comment on attachment 140049 [details] [diff] [review]
patch v2

r=relyea

Comment 22

[Julien Pierre]

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 .

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 26

[Wan-Teh Chang]

Attachment: Patch for the trunk

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

Comment 27

[Wan-Teh Chang]

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

[Wan-Teh Chang]

Attachment: 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

[Julien Pierre]

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 .

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

I'm marking this bug fixed again.  The fix will be in DBM in NSS 3.10