[Static Analysis] DEAD_STORE errors in security/sandbox/linux/*

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: rbartlensky, Assigned: rbartlensky)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

security/sandbox/linux/SandboxOpenedFiles.cpp:41: error: DEAD_STORE
  The value written to &fd (type int) is never used.
  39.   SandboxOpenedFile::GetDesc() const
  40.   {
  41. >   int fd = -1;
  42.     if (mDup) {
  43.       fd = mMaybeFd;


security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:127: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  125.     bool more = true;
  126.     do {
  127. >     rv = lineStream->ReadLine(line, &more);
  128.       // Cut off any comments at the end of the line, also catches lines
  129.       // that are entirely a comment
Assignee

Updated

11 months ago
Summary: [Static Analysis] DEAD_STORE errors in security/sandbox/linux/SandboxOpenedFiles.cpp → [Static Analysis] DEAD_STORE errors in security/sandbox/linux/*
Comment hidden (mozreview-request)

Comment 2

11 months ago
mozreview-review
Comment on attachment 8992704 [details]
Bug 1476340: Fix DEAD_STORE errors in security/sandbox/linux/*.

https://reviewboard.mozilla.org/r/257554/#review264516

::: security/sandbox/linux/SandboxOpenedFiles.cpp:41
(Diff revision 1)
>  }
>  
>  int
>  SandboxOpenedFile::GetDesc() const
>  {
> -  int fd = -1;
> +  int fd = 0;

If it's really necessary to do this, then just remove the initializer; `0` is not a safe default value for a file descriptor, and using it as such risks introducing bugs if this code is changed.

Having no initializer would risk introducing a use-uninitialized bug instead, but I believe we have enough static checking on CI (thinking of `-Werror` here) to detect at least the simple cases, and this code runs inside the sandbox so there's mitigation if the worst happens.
Attachment #8992704 - Flags: review-

Comment 3

11 months ago
mozreview-review
Comment on attachment 8992704 [details]
Bug 1476340: Fix DEAD_STORE errors in security/sandbox/linux/*.

https://reviewboard.mozilla.org/r/257554/#review264698

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:128
(Diff revision 1)
>      return;
>    }
>    nsAutoCString line;
>    bool more = true;
>    do {
> -    rv = lineStream->ReadLine(line, &more);
> +    mozilla::Unused << lineStream->ReadLine(line, &more);

We should probably exit the loop if ReadLine errors out for whatever reason.
Attachment #8992704 - Flags: review?(gpascutto) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

11 months ago
mozreview-review
Comment on attachment 8992704 [details]
Bug 1476340: Fix DEAD_STORE errors in security/sandbox/linux/*.

https://reviewboard.mozilla.org/r/257554/#review265162
Attachment #8992704 - Flags: review?(gpascutto) → review+
Assignee

Updated

11 months ago
Keywords: checkin-needed

Comment 7

11 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/564e53c57905
Fix DEAD_STORE errors in security/sandbox/linux/*. r=gcp
Keywords: checkin-needed

Comment 8

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/564e53c57905
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.