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

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: rbartlensky, Assigned: rbartlensky)

Tracking

(Blocks: 1 bug)

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
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

5 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 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

5 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

5 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

5 months ago
Keywords: checkin-needed

Comment 7

5 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

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