1070493 - file descriptor leak in "secu_InitSlotPassword" in 'nss/cmd/lib/secutil.c'

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Sachin Kumar Gupta]

Attachment: Patch for the above raised bug

File in which bug exists : nss/cmd/lib/secutil.c

Function name in which bug exists: secu_InitSlotPassword

The file descriptor 'input' has not been taken care of, when file descriptor 'output' is NULL and function terminates due to the return statement (line 293). 


Before calling return in 'output' NULL check, file descriptor input should be closed using fclose().

NSS 3.16.4 version code::

if (input == NULL) {
        PR_fprintf(PR_STDERR, "Error opening input terminal for read\n");
        return NULL;
    }

    /* we have no password, so initialize database with one */
    PR_fprintf(PR_STDERR,
        "Enter a password which will be used to encrypt your keys.\n"
        "The password should be at least 8 characters long,\n"
        "and should contain at least one non-alphabetic character.\n\n");

    output = fopen(consoleName, "w");
    if (output == NULL) {
        PR_fprintf(PR_STDERR, "Error opening output terminal for write\n");
        return NULL;
    }

Recommended code:

if (input == NULL) {
        PR_fprintf(PR_STDERR, "Error opening input terminal for read\n");
        return NULL;
    }

    /* we have no password, so initialize database with one */
    PR_fprintf(PR_STDERR,
        "Enter a password which will be used to encrypt your keys.\n"
        "The password should be at least 8 characters long,\n"
        "and should contain at least one non-alphabetic character.\n\n");

    output = fopen(consoleName, "w");
    if (output == NULL) {
        PR_fprintf(PR_STDERR, "Error opening output terminal for write\n");
#ifndef _WINDOWS
        fclose(input);
#endif
        return NULL;
    }

Comment 1

[Sachin Kumar Gupta]

Reminder for bug and patch review.

Comment 2

[Sachin Kumar Gupta]

Reminder for bug review.

Comment 3

[Robert Relyea]

Comment on attachment 8492584 [details] [diff] [review]
Patch for the above raised bug

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

r+ rrelyea

Comment 4

[Sachin Kumar Gupta]

Reminder for code change check in.

Comment 5

[Sachin Kumar Gupta]

Reminder for code check in.

Comment 6

[Kai Engert (:KaiE:)]

Thank you for your patch and the reminder!

Checked in:
https://hg.mozilla.org/projects/nss/rev/e982d1ffdca7

Comment 7

[Wan-Teh Chang]

Comment on attachment 8492584 [details] [diff] [review]
Patch for the above raised bug

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

::: cmd/lib/secutil.c
@@ +290,5 @@
>      output = fopen(consoleName, "w");
>      if (output == NULL) {
>  	PR_fprintf(PR_STDERR, "Error opening output terminal for write\n");
> +#ifndef _WINDOWS
> +	fclose(input);

It seems that the preexisting fclose(input) call at the end
of secu_InitSlotPassword should also be put inside #ifndef _WINDOWS.

Should we make that change?