1003929 - Clang Static Analysis: useless declarations in netwerk

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Attachment: netwerk-minor-fix.diff

The attached patch fixes some warnings detected by scan-build, the clang static analyzer.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: netwerk-minor-fix-2.diff

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Attachment: netwerk-minor-fix-3.diff

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Can you please give these patches unique commit messages?
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 4

[Michal Novotny [:michal]]

Comment on attachment 8415436 [details] [diff] [review]
netwerk-minor-fix-3.diff

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -1634,5 @@
>  
>      // step 1
>      // If connection pressure, then we want to favor pipelining of any kind
>      if (IsUnderPressure(ent, classification) && !attemptedOptimisticPipeline) {
> -        attemptedOptimisticPipeline = true;

This seems to be wrong. Useless assignment is below in step 3 and not here.

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8415436 [details] [diff] [review]
netwerk-minor-fix-3.diff

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -1634,5 @@
>  
>      // step 1
>      // If connection pressure, then we want to favor pipelining of any kind
>      if (IsUnderPressure(ent, classification) && !attemptedOptimisticPipeline) {
> -        attemptedOptimisticPipeline = true;

michal's right - that's necessary afaict

Comment 6

[Patrick McManus [:mcmanus]]

:sylvestre, thanks for doing this.

I'm going to ask that you reorganize these patches so they can get reviews by the people actively working on that particular piece of code.

for sctp/* ask :jesup
for srtp/* ask :sworkman
for rest ask me and I'll reroute as necessary.

I'll clear the reviews in hopes of lessening the confusion.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Attachment: bug-1003929.diff

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Attachment: bug-1003929-2.diff

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Attachment: bug-1003929-3.diff

Patrick, here are the three patches split as you asked.
In -3, I fixed the mistake reported in comment #4.

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8417906 [details] [diff] [review]
bug-1003929-3.diff

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

in each case (especially trydispatchconnection) the assignment reflects a state change. If there were a new step added to the end of the function it would reference that state to do the right thing - you can already see that pattern exists with attemptedOptimisticPipeline.

So I prefer to leave the code the way it is in order to minimize future errors.

Is there a suppression we can give clang to tell it this is not an accident?

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Attachment: bug-1003929-3.diff

Make sense, removed.

AFAIK, there is no easy way in clang analyzer to silent this warning (besides ugly ifdef).

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8417903 [details] [diff] [review]
bug-1003929.diff

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

The three sctp/src changes are in upstream code, and likely are needed on other uses (like BSD kernel) and so should be ifdefed.  Better would be to patch them upstream and import that patch.  Michael Tuexen can facilitate that.

The changes are likely ok/dafe, but I prefer to keep us 'clean' as possible against upstream to ease import, and fixing upstream and importing will avoid having to back out our changes later.

Land DataChannel.cpp, and wait on the others

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ -710,5 @@
>      res = peer->SendPacket(static_cast<unsigned char *>(buffer), length, false);
>    } else {
>      unsigned char *data = new unsigned char[length];
>      memcpy(data, buffer, length);
> -    res = -1;

This is there to document what we'd like to have occur (get a return value from SendPacket).  Please leave it there with a comment "Commented out since we have to Dispatch SendPacket to avoid deadlock"

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Attachment: bug-1003929.diff

Patch updated with Randall's comment.

Comment 14

[Michael Tüxen]

The problems in usrsctp have been resolved upstream:
https://bugzilla.mozilla.org/show_bug.cgi?id=1003929

For one of the problems, there were more changes required:
https://code.google.com/p/sctp-refimpl/source/diff?spec=svn8868&old=8866&r=8868&format=unidiff&path=%2Ftrunk%2FKERN%2Fusrsctp%2Fusrsctplib%2Fnetinet%2Fsctp_pcb.c

Thanks for reporting the issue.

Comment 15

[Michael Tüxen]

Comment on attachment 8418076 [details] [diff] [review]
bug-1003929.diff

See the upstream changes in
https://code.google.com/p/sctp-refimpl/source/detail?r=8868
Only the change in sctp_pcb.c requires more changes (which are relevant on
other platforms).
Thanks for reporting.

Best regards
Michael

Comment 16

[Sylvestre Ledru [:Sylvestre]]

thanks Michael, I wasn't aware it was a thirdparty dependency.

Comment 17

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8417904 [details] [diff] [review]
bug-1003929-2.diff

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

:rjesup should look at these too. These are srtp files to do with WebRTC (I think), but not rtsp.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8417904 [details] [diff] [review]
bug-1003929-2.diff

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

These need to be resolved with upstream first, and then import a stable release from upstream

::: netwerk/srtp/src/crypto/replay/rdbx.c
@@ -138,5 @@
> -  uint16_t guess_seq = (uint16_t) (low32(*guess));
> -#else
> -  uint32_t guess_roc = (uint32_t)(*guess >> 16);
> -  uint16_t guess_seq = (uint16_t) *guess;  
> -#endif

This file is ... confusing.  it needs a deeper look upstream (and check to see if upstream has been modified here).

::: netwerk/srtp/src/srtp/ekt.c
@@ -165,5 @@
>  			  unsigned pkt_octet_len) {
>    err_status_t err;
>    const uint8_t *master_key;
>    srtp_policy_t srtp_policy;
> -  unsigned master_key_len;

This file is fine

Comment 19

[Sylvestre Ledru [:Sylvestre]]

(In reply to Randell Jesup [:jesup] from comment #18)
> This file is ... confusing.  it needs a deeper look upstream (and check to
> see if upstream has been modified here).
Randell, this code does not exist anymore in upstream. Therefor, is r+ OK on Attachment #8417904 [details] [diff] ? Thanks

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8417904 [details] [diff] [review]
bug-1003929-2.diff

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

Might as well, if they're not in upstream.  (Depending on how we're carrying any patches to an upstream module, this sort of thing may or may not cause problems.  I think in this case it does not.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5a20d7f35fb
https://hg.mozilla.org/mozilla-central/rev/005fd57e65ae
https://hg.mozilla.org/mozilla-central/rev/0b6b34db9bf9