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

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P3)

Description

[Robert Bartlensky [:rbartlensky]]

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

Comment 1

[Robert Bartlensky [:rbartlensky]]

Attachment: Bug 1476340: Fix DEAD_STORE errors in security/sandbox/linux/*.

Review commit: https://reviewboard.mozilla.org/r/257554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257554/

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 4

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8992704 [details]
Bug 1476340: Fix DEAD_STORE errors in security/sandbox/linux/*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257554/diff/1-2/

Comment 5

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8992704 [details]
Bug 1476340: Fix DEAD_STORE errors in security/sandbox/linux/*.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257554/diff/2-3/

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8992704 [details]
Bug 1476340: Fix DEAD_STORE errors in security/sandbox/linux/*.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/564e53c57905
Fix DEAD_STORE errors in security/sandbox/linux/*. r=gcp

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/564e53c57905