1476640 - [Static Analysis] infer errors in media/mtransport/*.

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P3)

Description

[Robert Bartlensky [:rbartlensky]]

media/mtransport/nricectx.cpp:649: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  647.       rv = pref_service->GetBranch(nullptr, getter_AddRefs(pref_branch));
  648.       if (NS_SUCCEEDED(rv)) {
  649. >       rv = pref_branch->GetCharPref(
  650.             "media.peerconnection.nat_simulator.mapping_type",
  651.             mapping_type);

media/mtransport/nricectx.cpp:652: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  650.             "media.peerconnection.nat_simulator.mapping_type",
  651.             mapping_type);
  652. >       rv = pref_branch->GetCharPref(
  653.             "media.peerconnection.nat_simulator.filtering_type",
  654.             filtering_type);

media/mtransport/nricectx.cpp:655: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  653.             "media.peerconnection.nat_simulator.filtering_type",
  654.             filtering_type);
  655. >       rv = pref_branch->GetBoolPref(
  656.             "media.peerconnection.nat_simulator.block_udp",
  657.             &block_udp);

media/mtransport/transportlayerdtls.cpp:1272: error: DEAD_STORE
  The value written to &rv (type int) is never used.
  1270.
  1271.           // Checking functions call PR_SetError()
  1272. >         SECStatus rv = SECFailure;
  1273.           for (auto digest : digests_) {
  1274.             rv = CheckDigest(digest, peer_cert);

Comment 1

[Robert Bartlensky [:rbartlensky]]

Attachment: Bug 1476640: Fix DEAD_STORE errors in media/mtransport/*.

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

Comment 2

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8993001 [details]
Bug 1476640: Fix DEAD_STORE errors in media/mtransport/*.

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

Comment 3

[Adam Roach [:abr]]

Comment on attachment 8993001 [details]
Bug 1476640: Fix DEAD_STORE errors in media/mtransport/*.

https://reviewboard.mozilla.org/r/257812/#review264780

Looks good to me.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8993001 [details]
Bug 1476640: Fix DEAD_STORE errors in media/mtransport/*.

https://reviewboard.mozilla.org/r/257812/#review264972

::: media/mtransport/transportlayerdtls.cpp:1272
(Diff revision 2)
>        {
>          MOZ_ASSERT(digests_.size() != 0);
>          // Check all the provided digests
>  
>          // Checking functions call PR_SetError()
> -        SECStatus rv = SECFailure;
> +        SECStatus rv;

Sorry to be late to the party, but I think this is a mistaken change. I recognize that it's a dead store but it's better to think of it as just initialization. Is there a way to suppress this error?

Comment 5

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8993001 [details]
Bug 1476640: Fix DEAD_STORE errors in media/mtransport/*.

https://reviewboard.mozilla.org/r/257812/#review264972

> Sorry to be late to the party, but I think this is a mistaken change. I recognize that it's a dead store but it's better to think of it as just initialization. Is there a way to suppress this error?

As far as I know you can't silence warnings, only fix them. Two fixes that work are: initialize to something that is 0, in this case `SECSucess`, or do not initialize it.

Comment 6

[Eric Rescorla (:ekr)]

In that case, this seems like it's a bad piece of static analysis and we should consider disabling it.

Comment 7

[Cristina Coroiu [:ccoroiu]]

Please fix the remaining open issue, so we can land this bug.

Comment 8

[Robert Bartlensky [:rbartlensky]]

(In reply to Eric Rescorla (:ekr) from comment #6)
> In that case, this seems like it's a bad piece of static analysis and we
> should consider disabling it.

Then I will mark it as a false positive, and revert some of the changes.

Comment 9

[Robert Bartlensky [:rbartlensky]]

Comment on attachment 8993001 [details]
Bug 1476640: Fix DEAD_STORE errors in media/mtransport/*.

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

Comment 10

[Eric Rescorla (:ekr)]

Can you please upload this to Phabricator. We are using that now.

Comment 11

[Robert Bartlensky [:rbartlensky]]

Attachment: Bug 1476640: Fix DEAD_STORE errors in media/mtransport/*.

Comment 12

[Phabricator Automation]

Comment on attachment 8993661 [details]
Bug 1476640: Fix DEAD_STORE errors in media/mtransport/*.

Eric Rescorla (:ekr) has approved the revision.

https://phabricator.services.mozilla.com/D2265

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b952f123c7
Fix DEAD_STORE errors in media/mtransport/*.

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b8b952f123c7