Coverity bugs in modutil/install.c

RESOLVED FIXED in 3.27

Status

NSS
Libraries
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: fkiefer, Assigned: mt)

Tracking

(Blocks: 1 bug, {coverity})

trunk
3.27
coverity

Firefox Tracking Flags

(firefox46 affected)

Details

(Whiteboard: CID96021, CID96052, CID222191, CID982220, CID1364695)

Attachments

(1 attachment, 1 obsolete attachment)

There are multiple bugs coverity found in install.c (CID 96021, 96052, 222191, 982220).
(Assignee)

Comment 1

2 years ago
Created attachment 8777153 [details] [diff] [review]
bug1233020-1.patch

This seems the most conservative approach.  I chose to skip iterations of the loop where `dest` wasn't being assigned rather than error out.  This is probably OK because it would seem that this path was never hit in practice.
Assignee: franziskuskiefer → martin.thomson
Attachment #8777153 - Flags: review?(franziskuskiefer)
Comment on attachment 8777153 [details] [diff] [review]
bug1233020-1.patch

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

Thanks for picking this up!
I think we need a null check for reldir.

::: cmd/modutil/install.c
@@ +537,5 @@
>  
>      ret = PK11_INSTALL_SUCCESS;
>  loser:
>      if (Pk11Install_valueList) {
>          Pk11Install_ValueList_delete(Pk11Install_valueList);

interesting that this one wasn't picked up before...

@@ +603,5 @@
>          file = &platform->files[i];
>  
>          if (file->relativePath) {
>              PRBool foundMarker = PR_FALSE;
> +            char *reldir = PR_Strdup(file->relativePath);

if PR_Strdup fails, reldir could be NULL (when malloc fails). We'd free the NULL reldir and dest in this case (or more likely fail before).
Attachment #8777153 - Flags: review?(franziskuskiefer)
Whiteboard: CID96021, CID96052, CID222191, CID982220 → CID96021, CID96052, CID222191, CID982220, CID1364695
(Assignee)

Comment 3

2 years ago
Created attachment 8777264 [details] [diff] [review]
bug1233020-1.patch

I decided to log unspecified errors in both of the cases that things go off the rails.
Attachment #8777153 - Attachment is obsolete: true
Attachment #8777264 - Flags: review?(franziskuskiefer)
Attachment #8777264 - Flags: review?(franziskuskiefer) → review+
(Assignee)

Comment 4

2 years ago
https://hg.mozilla.org/projects/nss/rev/d85ec4efc9ec
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.27
You need to log in before you can comment on or make changes to this bug.