1174781 - PR_GetInheritedFD can use uninitialized variables

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[q1]

PR_GetInheritedFD (38.0.1\nsprpub\pr\src\misc\prinit.c line 624) fails to check the return value from PR_sscanf, but still uses the resulting uninitialized variables:

631:    PROsfd osfd;
632:    int nColons;
633:    PRIntn fileType;
...
645:            PR_sscanf(ptr, "%d:0x%" PR_SCNxOSFD, &fileType, &osfd);
646:            switch ((PRDescType)fileType) {
647:                case PR_DESC_FILE:
648:                    fd = PR_ImportFile(osfd);
649:                    break;
...                 ... // more of the same

As far as I can tell, PR_GetInheritedFD presently is called only from test code and a standalone utility:

  38.0.1\nsprpub\pr\tests\pipepong2.c(37):    pipe_read = PR_GetInheritedFD("PIPE_READ");
  38.0.1\nsprpub\pr\tests\pipepong2.c(39):        fprintf(stderr, "PR_GetInheritedFD failed\n");
  38.0.1\nsprpub\pr\tests\pipepong2.c(47):    pipe_write = PR_GetInheritedFD("PIPE_WRITE");
  38.0.1\nsprpub\pr\tests\pipepong2.c(49):        fprintf(stderr, "PR_GetInheritedFD failed\n");
  38.0.1\nsprpub\pr\tests\sockpong.c(37):    sock = PR_GetInheritedFD("SOCKET");
  38.0.1\nsprpub\pr\tests\sockpong.c(39):        fprintf(stderr, "PR_GetInheritedFD failed\n");
  38.0.1\security\nss\cmd\selfserv\selfserv.c(2411):	listen_sock = PR_GetInheritedFD(inheritableSockName);
  38.0.1\security\nss\cmd\selfserv\selfserv.c(2413):	    errExit("PR_GetInheritedFD");

so I guess that this bug doesn't presently cause an important security problem.

Comment 1

[Daniel Veditz [:dveditz]]

This is only used in the selfserv command-line utility in NSS, not part of Firefox.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch: check the return value of PR_sscanf

q1: thank you very much for the bug report. Please test
and review this patch.

Kai: PR_sscanf returns the number of values converted from
the input. We want to convert two values in this PR_sscanf
call, so I compare its return value with 2. On error, I am
just using the same error reporting code used by this
function, which sets a useless error code (PR_UNKNOWN_ERROR)
and returns NULL.

Comment 3

[q1]

(In reply to Wan-Teh Chang from comment #2)
> Created attachment 8624864 [details] [diff] [review]
> Proposed patch: check the return value of PR_sscanf
> 
> q1: thank you very much for the bug report. Please test
> and review this patch.

The patch looks correct. I don't have a testbench setup, so I haven't tested it.

Comment 4

[q1]

Comment on attachment 8624864 [details] [diff] [review]
Proposed patch: check the return value of PR_sscanf

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

I didn't notice this tool at first. The patch looks OK. I don't have a testbench setup, so I haven't tested it.

Comment 5

[q1]

How do I turn off the feedback request flag?

Comment 6

[Wan-Teh Chang]

q1: click the "Review" link to the right of the attachment named
"Proposed patch: check the return value of PR_sscanf". For your
convenience, you can reach there with this link:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1174781&attachment=8624864

Then, change the '?' for feedback to a space (which cancels/clears
the feedback request), '+' (which indicates your approval), or
'-' (which indicates your disapproval). You can add an optional
comment in the text field. Then Click the "Publish" button.

Comment 7

[q1]

Thanks.

Comment 8

[Wan-Teh Chang]

Comment on attachment 8624864 [details] [diff] [review]
Proposed patch: check the return value of PR_sscanf

Kai, Ted: please review this patch. Thanks.

Comment 9

[Wan-Teh Chang]

Comment on attachment 8624864 [details] [diff] [review]
Proposed patch: check the return value of PR_sscanf

https://hg.mozilla.org/projects/nspr/rev/e2177b7f79f5