135292 - unsigned int being compared to -1

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[anthonyd]

actual code in question:
ns4xPluginInstance.cpp, line 252
  if (mNPStream.end == -1)
    mNPStream.end = 0;

getting set to -1 in nsPluginHostImpl.cpp, line 2103.

Fix or not? seems like we should, but I'm cold and scared.

anthonyd

Comment 1

[anthonyd]

Attachment: patch to fix the bug

Comment 2

[serge (gone)]

Tony, did you check the test cases from bug 52963, bug 52965 ?
it's about java servlet connectivity through nsIPluginManager::GetURL/PostURL
are there any reasons for that code?
+  // it's possible for the server to not send a Content-Length.  We should
+  // still work in this case.
+  if (NS_FAILED(rv)) {
+    mPluginStreamInfo->SetLength(-1);

Comment 3

[serge (gone)]

BTW: is there a posibility to get -1 from channel ?
  rv = channel->GetContentLength(&length);
  // it's possible for the server to not send a Content-Length.  We should
  // still work in this case.
  if (NS_FAILED(rv)) {
    mPluginStreamInfo->SetLength(-1);
  }
  else {
    mPluginStreamInfo->SetLength(length);
  }

Comment 4

[anthonyd]

Its not so much about whether there is a reason for the code as it is about
whether the code is correct. In the patch I attached, mNPStream.end (which is an
unsigned int32) get assinged a -1 with the following block:

nsPluginHostImpl.cpp
    mPluginStreamInfo->SetLength(-1);

at some point later in the code, it is compared to -1 and set to zero if true:

ns4xPluginInstance.cpp
  if (mNPStream.end == -1)
    mNPStream.end = 0;

That variable is only set to -1 (really just FFFFF....) in one place, so after
talking with peterl, the safe thing seemed to be to just go ahead and assign it
to zero, since that seemed to be the purpose anyway of the code.

It appeared to be just blind luck that the code was working in the first place,
if that had been an unsigned short for instance, it would have failed.

As to the testcases you mention, I will rebuild and test them, but peterl would
be a better choice to ask about further ramifications of this small change.

anthonyd

Comment 5

[Peter Lubczynski]

I'm just worried that someone may be expecting this -1 (FFFFFF) behavior.

Comment 6

[anthonyd]

patch attached.

Comment 7

[rubydoo123]

1.2beta

Comment 8

[rubydoo123]

adding nsbeta1+ for buffy

Comment 9

[anthonyd]

need r's and sr's for this patch.

Comment 10

[av (gone)]

Comment on attachment 78462 [details] [diff] [review]
patch to fix the bug

The code seems to be logically equivalent to what it was. r=av. Maybe it would
be nicer to write it in one line:

mPluginStreamInfo->SetLength(NS_SUCCEEDED(rv) ? length : 0);

and make sure that testcases from all related bugs still work.

Comment 11

[av (gone)]

Comment on attachment 78462 [details] [diff] [review]
patch to fix the bug

Serge just pointed out... We probably don't want commented out source lines
dangling in the code, please make sure you remove them before you check in. Or
better, attach a new patch.

Comment 12

[serge (gone)]

Attachment: alt patch

I wouldn't remove this due to next comment
   // if we don't know the end of the stream, use 0 instead of -1. bug 59571
-  if (mNPStream.end == -1)
+  if ((int)mNPStream.end == -1)

Comment 13

[av (gone)]

Comment on attachment 94718 [details] [diff] [review]
alt patch

Serge, but in the original patch, we already set it to null if we don't know
the length.

Comment 14

[serge (gone)]

Attachment: alt v2

all right after conversation with av we decided it safe now to remove 
if (mNPStream.end == -1)
from ns4xPluginStreamListener::OnStartBinding

Comment 15

[serge (gone)]

oops, wrong attachment:(

Comment 16

[serge (gone)]

Attachment: alt v3

it's a right one now

Comment 17

[av (gone)]

Comment on attachment 94724 [details] [diff] [review]
alt v3

r=av

Comment 18

[anthonyd]

I don't have a problem with the new patch, but who do I get a sr from?

Comment 19

[Darin Fisher]

Comment on attachment 94724 [details] [diff] [review]
alt v3

>Index: nsPluginHostImpl.cpp

>+  PRInt32 length = 0;
>+  channel->GetContentLength(&length);
>+  mPluginStreamInfo->SetLength(length);

you must check the return value of GetContentLength.  it might fail,
and upon failing it might set length to some arbitrary value that
isn't at all consistent with the concept of content length.

Comment 20

[serge (gone)]

well, I thought idl out param suppose to be unattached or set to 0 on failure.
but it looks like there is no such requirements for out params:(
lets use av's patch from comment #10
PRInt32 length;
rv = channel->GetContentLength(&length)
mPluginStreamInfo->SetLength(NS_SUCCEEDED(rv) ? length : 0);

Comment 21

[anthonyd]

Though it probably doesn't matter for this particular case, I really don't like
the trinary argument because it is harder to debug (putting breakpoints).  There
is also no real point because any decent compiler will make sure that a trinary
is equivalent to a if/else block.

Anyway, whichever patch you want, just let me know.

ADavis

Comment 22

[serge (gone)]

please attach the patch you like.

Comment 23

[anthonyd]

I think the original patch was the simplest and most straight forward (other
than the commented out lines of code, which I should have just deleted).

I will go ahead and reattach the patch WITHOUT the commented out lines of code.
 But is there a technical error with the original patch?

Thanks,
ADavis

Comment 24

[serge (gone)]

Comment on attachment 78462 [details] [diff] [review]
patch to fix the bug

the patch looks good, except the comments
>+  //if (mNPStream.end == -1)
>+  //  mNPStream.end = 0; // see bug # 135292
>...
>+//    mPluginStreamInfo->SetLength(-1);
>+    mPluginStreamInfo->SetLength(0);// see bug # 135292

Comment 25

[anthonyd]

Attachment: new version of original patch

this is the simple patch with the comments removed.  just need the r= and sr=
to get this finsihed.

thanks all.

Comment 26

[serge (gone)]

Comment on attachment 95470 [details]
new version of original patch

r=serge

Comment 27

[Darin Fisher]

Comment on attachment 95470 [details]
new version of original patch

sr=darin

Comment 28

[anthonyd]

got everything I need, will check in ASAP.

thanks everyone.
anthonyd

Comment 29

[anthonyd]

checked in to trunk.

Comment 30

[shrirang khanzode]

.