1234524 - [CID 221866][CID 748522] unchecked return values in sdb.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

Unchecked return values in sdb_init and sdb_measureAccess in sdb.c

Comment 1

[Michelangelo De Simone [Offline] [:mds]]

Hey Franziskus would you please paste Coverity's lament here?:)

Comment 2

[Franziskus Kiefer [:franziskus]]

369    /* no directory, just return one */
   1. Condition directory == NULL, taking false branch
370    if (directory == NULL) {
371        return 1;
372    }
373
374    /* our calculation assumes time is a 4 bytes == 32 bit integer */
   2. Condition 1 /* sizeof (time) == 4 */, taking true branch
375    PORT_Assert(sizeof(time) == 4);
376
377    directoryLength = strlen(directory);
378
379    maxTempLen = directoryLength + strlen(doesntExistName)
380                 + 1 /* potential additional separator char */
381                 + 11 /* max chars for 32 bit int plus potential sign */
382                 + 1; /* zero terminator */
383
384    temp = PORT_Alloc(maxTempLen);
   3. Condition !temp, taking false branch
385    if (!temp) {
386        return 1;
387    }
388
389    /* We'll copy directory into temp just once, then ensure it ends
390     * with the directory separator, then remember the position after
391     * the separator, and calculate the number of remaining bytes. */
392
393    strcpy(temp, directory);
   4. Condition directory[directoryLength - 1] != PR_GetDirectorySeparator(), taking true branch
394    if (directory[directoryLength - 1] != PR_GetDirectorySeparator()) {
395        temp[directoryLength++] = PR_GetDirectorySeparator();
396    }
397    tempStartOfFilename = temp + directoryLength;
398    maxFileNameLen = maxTempLen - directoryLength;
399
400    /* measure number of Access operations that can be done in 33 milliseconds
401     * (1/30'th of a second), or 10000 operations, which ever comes first.
402     */
403    time =  PR_IntervalNow();
   5. Condition i < 10000U, taking true branch
404    for (i=0; i < 10000u; i++) { 
405        PRIntervalTime next;
406
407        /* We'll use the variable part first in the filename string, just in
408         * case it's longer than assumed, so if anything gets cut off, it
409         * will be cut off from the constant part.
410         * This code assumes the directory name at the beginning of
411         * temp remains unchanged during our loop. */
412        PR_snprintf(tempStartOfFilename, maxFileNameLen,
413                    ".%lu%s", (PRUint32)(time+i), doesntExistName);
   CID 221866 (#1 of 1): Unchecked return value (CHECKED_RETURN)6. check_return: Calling PR_Access without checking return value (as is done elsewhere 14 out of 15 times).
414        PR_Access(temp,PR_ACCESS_EXISTS);
415        next = PR_IntervalNow();
416        delta = next - time;
417        if (delta >= duration)
418            break;
419    }


1722    LOCK_SQLITE();
    1. Condition PR_Access(dbname, PR_ACCESS_EXISTS) != PR_SUCCESS, taking true branch
1723    create = (PR_Access(dbname, PR_ACCESS_EXISTS) != PR_SUCCESS);
    2. Condition flags == 1, taking false branch
1724    if ((flags == SDB_RDONLY) && create) {
1725        error = sdb_mapSQLError(type, SQLITE_CANTOPEN);
1726        goto loser;
1727    }
1728    sqlerr = sdb_openDB(dbname, &sqlDB, flags);
    3. Condition sqlerr != 0, taking false branch
1729    if (sqlerr != SQLITE_OK) {
1730        error = sdb_mapSQLError(type, sqlerr); 
1731        goto loser;
1732    }
1733    /* sql created the file, but it doesn't set appropriate modes for
1734     * a database */
    4. Condition create, taking true branch
1735    if (create) {
1736        /* NO NSPR call for this? :( */
    CID 748522 (#1 of 1): Unchecked return value from library (CHECKED_RETURN)5. check_return: Calling chmod(dbname, 384U) without checking return value. This library function may fail and return an error code.
1737        chmod (dbname, 0600);
1738    }

Comment 3

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Fixing a potentially unsafe chmod call in sdb.c

Comment 4

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8736039 [details] [diff] [review]
Fixing a potentially unsafe chmod call in sdb.c

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #2)

>    CID 221866 (#1 of 1): Unchecked return value (CHECKED_RETURN)6.
> check_return: Calling PR_Access without checking return value (as is done
> elsewhere 14 out of 15 times).
> 414        PR_Access(temp,PR_ACCESS_EXISTS);

Hey Franziskus,

this call here is the actuall core of the function and pretty clearly a false positive: we only need to execute the function to check its running time but we're not interested in its result. We could of course have a PR_Bool but I'm unsure whether putting something else on the frame would make sense.

Is there a way to mark this as a false-positive in Coverity?

>     CID 748522 (#1 of 1): Unchecked return value from library
> (CHECKED_RETURN)5. check_return: Calling chmod(dbname, 384U) without
> checking return value. This library function may fail and return an error
> code.
> 1737        chmod (dbname, 0600);
> 1738    }

OTOH this looks legit: I've embedded the function within a condition; what do you think about it?

I didn't put the patch up for review yet because of the previous false-positive.

Comment 5

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8736039 [details] [diff] [review]
Fixing a potentially unsafe chmod call in sdb.c

Review of attachment 8736039 [details] [diff] [review]:
-----------------------------------------------------------------

the second part looks good to me.

to the first:
> > >    CID 221866 (#1 of 1): Unchecked return value (CHECKED_RETURN)6.
> > check_return: Calling PR_Access without checking return value (as is done
> > elsewhere 14 out of 15 times).
> > 414        PR_Access(temp,PR_ACCESS_EXISTS);
> 
> this call here is the actuall core of the function and pretty clearly a false positive: we only need to execute the function to check its running time but we're not interested in its result. We could of course have a PR_Bool but I'm unsure whether putting something else on the frame would make sense.

yes we can mark things as false positive but I'm not sure if this is the case here (I'd prefer a check but read on). If access fails PR_Access returns PR_FAILURE. However, I'm not entirely sure what to do if that happens. Without checking the return value the call is a bit pointless. It looks like the purpose of the function is to measure how fast the access is.
This is only used in [1], which compares two values produced by this function. So adding a check for the result shouldn't hurt but I don't know what to do after checking it. Breaking would skew the result. If you don't have a better idea I'll mark it as false positive.

[1] https://dxr.mozilla.org/nss/source/nss/lib/softoken/sdb.c#1887

Comment 6

[Michelangelo De Simone [Offline] [:mds]]

sdb_measureAccess is also being used in s_open(...) (other than in sdb_init(...)).

The sole purpose of the function is measuring the average access times for non-existing files: it does that - if I understood - by counting how many new files it can access(2) in 33ms.

Hence, the higher the return of sdb_measureAccess, the faster the performance => the more convenient the proposed directory.

The whole point is for access(2) to fail: if - by any chance - the function returns 0, it'd mean that sdb_measureAccess() would have hit an existing file, invalidating its semantics.

If I got the picture right, my suggestion is: check for PR_Access():414 return value, and if IT SUCCEEDS (unlikely), return 1u to signal a bad choice. No impact on the callers.

Otherwise we can slightly change its syntax by using the usual POSIX way and have it return negative values in case of unforeseen errors, and check those cases in the two callers in sdb.c.

(Or just mark it as false-positive and forget it :-))

Comment 7

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8736039 [details] [diff] [review]
Fixing a potentially unsafe chmod call in sdb.c

Review of attachment 8736039 [details] [diff] [review]:
-----------------------------------------------------------------

marked CID 221866 as intentional. let's land this one

Comment 8

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/6c3d7cb5178a

Comment 9

[Franziskus Kiefer [:franziskus]]

I should've tested before. This fails many tests. I suspect that chmod actually fails here but we never cared.

backed out: https://hg.mozilla.org/projects/nss/rev/2ab0ad981a4b

Comment 10

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #9)

> I should've tested before. This fails many tests. I suspect that chmod
> actually fails here but we never cared.

It wasn't up for review yet, indeed.;):)
I'm gonna check it and upload a formal patch.

Comment 11

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #9)
> I should've tested before. This fails many tests. I suspect that chmod
> actually fails here but we never cared.

I've just run the whole suite locally and there's no failure, at least locally: would you please point me to what failure you saw?

I've run everything on a Linux x86_64 with USE_64.

Comment 12

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Fixing a potentially unsafe chmod call in sdb.c

Comment 13

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8737463 [details] [diff] [review]
Fixing a potentially unsafe chmod call in sdb.c

(In reply to Michelangelo De Simone from comment #11)

> I've just run the whole suite locally and there's no failure

Nevermind, I was wrong: I had at least 30 tests failing too.
Let me know what you think about this.

Thanks!

Comment 14

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8737463 [details] [diff] [review]
Fixing a potentially unsafe chmod call in sdb.c

Review of attachment 8737463 [details] [diff] [review]:
-----------------------------------------------------------------

this looks better and passes all tests for me.

Comment 15

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #14)

> this looks better and passes all tests for me.

Thanks!

Comment 16

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/c719ec548096

Comment 17

[Kai Engert (:KaiE:)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #16)
> https://hg.mozilla.org/projects/nss/rev/c719ec548096

This is suspected to be causing Android test failures, see this try build, which has crashes in softokn:
https://hg.mozilla.org/projects/nss/rev/c719ec548096

I propose to backout, reopen and prior to committing again, verify the effects of the fix with a Firefox try build.

Comment 18

[Kai Engert (:KaiE:)]

backed out:
https://hg.mozilla.org/projects/nss/rev/e2ec1315ba8d

It seems the chmod call has unexpected behaviour on Android.

Comment 19

[Wan-Teh Chang]

Comment on attachment 8737463 [details] [diff] [review]
Fixing a potentially unsafe chmod call in sdb.c

Review of attachment 8737463 [details] [diff] [review]:
-----------------------------------------------------------------

I found a bug that is irrelevant if we decide to ignore
the failure of the chmod() call. If we handle chmod()
failure, we need to set |error| before goto loser.

::: lib/softoken/sdb.c
@@ +1737,5 @@
> +     *
> +     * NO NSPR call for chmod? :(
> +     */
> +    if (create && chmod(dbname, 0600) != 0) {
> +        goto loser;

BUG: at this point, |error| still has the initial value
of CKR_OK. Although chmod() is not an SQLite function, we
can simulate an SQLite error here:

    if (create && chmod(dbname, 0600) != 0) {
        error = sdb_mapSQLError(type, SQLITE_CANTOPEN);
        goto loser;
    }

or we can directly set |error| to an appropriate PKCS #11 error:

    if (create && chmod(dbname, 0600) != 0) {
        error = CKR_DEVICE_ERROR;
        goto loser;
    }

Comment 20

[Kai Engert (:KaiE:)]

Comment on attachment 8737463 [details] [diff] [review]
Fixing a potentially unsafe chmod call in sdb.c

Thanks Wan-Teh.

I assumed you intended to set this r- (not r+)

Comment 21

[Wan-Teh Chang]

Kai: yes, thank you.

Comment 22

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Fixing a potentially unsafe chmod call in sdb.c

Comment 23

[Wan-Teh Chang]

Comment on attachment 8738725 [details] [diff] [review]
Fixing a potentially unsafe chmod call in sdb.c

Review of attachment 8738725 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

Comment 24

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Wan-Teh Chang from comment #23)

> r=wtc.

Thanks! I'll finish running the test suite before putting it up for checkin.:)

Comment 25

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Fixing a potentially unsafe chmod call in sdb.c

Comment 26

[Franziskus Kiefer [:franziskus]]

A try run to see if FF on Android is ok with this

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73a7a7b3426

Comment 27

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #26)

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73a7a7b3426

The Linux builds are busted but I'm not sure why...

Comment 28

[Franziskus Kiefer [:franziskus]]

(In reply to Michelangelo De Simone from comment #27)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #26)
> 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73a7a7b3426
> 
> The Linux builds are busted but I'm not sure why...

That's probably my fault of not properly merging NSS into m-c... (I tripped over the new libfreeblpriv). But the Android tests look good already. Here another one that looks even better https://treeherder.mozilla.org/#/jobs?repo=try&revision=bedf8d50fa3a

Comment 29

[Kai Engert (:KaiE:)]

What's the status of this bug? Is the keyword "checkin-needed" correct? Or is this already resolved? Can you please fix the status? Thank you.

Comment 30

[Michelangelo De Simone [Offline] [:mds]]

AFAIK this should be good to go.

Comment 31

[Wan-Teh Chang]

Comment on attachment 8738781 [details] [diff] [review]
Fixing a potentially unsafe chmod call in sdb.c

Review of attachment 8738781 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. I suggest returning a more informative error code
than CKR_DEVICE_ERROR by faking the SQLITE_CANTOPEN error.

::: lib/softoken/sdb.c
@@ +1737,5 @@
> +     *
> +     * NO NSPR call for chmod? :(
> +     */
> +    if (create && chmod(dbname, 0600) != 0) {
> +        error = CKR_DEVICE_ERROR;

I suggest faking the SQLite error SQLITE_CANTOPEN here:

    error = sdb_mapSQLError(type, SQLITE_CANTOPEN);

This will allow us to return a more informative error
code (CKR_NETSCAPE_CERTDB_FAILED or CKR_NETSCAPE_KEYDB_FAILED)
than CKR_DEVICE_ERROR.

Comment 32

[Franziskus Kiefer [:franziskus]]

sorry, forgot to land this one. Landed as https://hg.mozilla.org/projects/nss/rev/bc6d0f4b167b

Michelangelo, do you want to do a follow up for this (sorry saw the comment only after pushing)

Comment 33

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #32)

> Michelangelo, do you want to do a follow up for this (sorry saw the comment
> only after pushing)

I'll follow-up Wan-Teh Chang's suggestion with an incremental change.

Comment 34

[Michelangelo De Simone [Offline] [:mds]]

Attachment: More informative error in sdb.c

Comment 35

[Wan-Teh Chang]

Comment on attachment 8747954 [details] [diff] [review]
More informative error in sdb.c

Review of attachment 8747954 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/softoken/sdb.c
@@ +1741,5 @@
> +        /*
> +         * A fake SQLITE_CANTOPEN error here sounds like a more
> +         * informative error for the user. See:
> +         * https://bugzilla.mozilla.org/show_bug.cgi?id=1234524#c31
> +         */

This comment isn't necessary, because we're already faking
the SQLITE_CANTOPEN error in two other places in this function.

Comment 36

[Michelangelo De Simone [Offline] [:mds]]

Attachment: More informative error in sdb.c

Comment 37

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8747996 [details] [diff] [review]
More informative error in sdb.c

Incremental change proposed and r='ed by wtc.
Franziskus, would you please check this one in too before marking the bug FIXED (granted everything is alright)?

Thanks!

Comment 38

[Wan-Teh Chang]

Comment on attachment 8747996 [details] [diff] [review]
More informative error in sdb.c

Review of attachment 8747996 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Thanks.

Comment 39

[Franziskus Kiefer [:franziskus]]

thanks, landed as https://hg.mozilla.org/projects/nss/rev/5b1295e84637