115308 - [viewpoint] HTTP POST does not handle binary data

Status: VERIFIED FIXED

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

Description

[scott dreier]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0; YUBC VeXi; 
T312461)
BuildID:    Mozilla 5.0 Gecko/20011019

When using NPN_PostURLNotify() and sending "\x0Hello World" as content, the 
content is not sent.  This works fine on other browsers (ie5.0,ie5.5,ie6.0, 
NS4.x).  If I ommit the leading 0, it works fine, but then it's not binary data 
anymore.

Reproducible: Always
Steps to Reproduce:
1.Set up a capture program to view TCP/IP transactions
2.Send some binary POST data.
3.

Comment 1

[Arun Ranganathan]

OK, there's something to this bug, and since it's a blocker for Flash, I'm
adding all the keywords.  NPN_PostURLNotify( ) is NPAPI stuff -- Darin, if you
rule out Necko, then we ought to hand this one off.  But I'm slapping an
edt0.9.4 and a mozilla0.9.7 since the importance of this bug warrants the keywords.

Comment 2

[Arun Ranganathan]

Argh, I'm also nsbranching this one.  Shoot me if nsbranch is a relic.

Comment 3

[Darin Fisher]

this is a problem in the plugins code... over to plugins.

Comment 4

[Peter Lubczynski]

Scott, I just fixed this in bug 108966. Please try a build from this afternoon
or Monday morning.

*** This bug has been marked as a duplicate of 108966 ***

Comment 5

[shrirang khanzode]

v d

Comment 6

[Judson Valeski]

minusing, this was fixed elsewhere.

Comment 7

[jody]

After talking with Shrirang, since this (115308) was marked dup of 108966,
108966 got fixed,but this one still doesn't work, so this is not duplicated with
108966. I  try to post http requests to Jrun server, didn't see any requests
through tcpmon, so reopen it.

Thanks.
Jody

Comment 8

[Peter Lubczynski]

Attachment: testcase showing binary data posting via files and raw data

Jody, can you post a link to your testcase or attach it here? It seems to be
working for me but there may have been a quirk in 4.x that I missed...

I've attached a testcase with two tester plugins showing us POSTing binary
data. Use only one at a time.

Comment 9

[jody]

Attachment: it has NPSWF32.dll(flash6 plugin for netscape), a test flash6 movie,a text file,a readme file.

a testcase showing no http request is post.

Comment 10

[Peter Lubczynski]

-->peterl and renominating....

Comment 11

[Peter Lubczynski]

missed mozilla0.9.7 but still edt0.9.4

Comment 12

[Michael Dunn]

Will plus 094 once fixed and verified om the trunk.

Comment 13

[Peter Lubczynski]

Jody,

The zip file you attached was missing the movie file (bug.swf) and I saw no
NPN_PostURL calls. Could you double check the testcase to make sure everything
is there.

Also, it possible, it would really help if you could look in the debugger and
take note of the values of the arguments being passed to NPN_PostURL. Knowing
that the exact arguments would really help in reproducing. Thanks!

Comment 14

[Peter Lubczynski]

-->av

Comment 15

[jody]

Attachment: This is a new attachment(please discard attachment for 62207). it has NPSWF32.dll(flash6 plugin for netscape), a test flash6 movie,a text file,a readme file.

Comment 16

[serge (gone)]

with test case from attachment 63128 [details] 
when I stop into
_posturlnotify(_NPP * 0x01057750, const char * 0x00186c38, const char * 
0x00000000, unsigned long 402, const 
char * 0x03ce2110, unsigned char 0, void * 0x00000003) line 765 + 56 bytes
NPSWF32_6! NP_Shutdown + 185 bytes
NPSWF32_6! NP_Shutdown + 9683 bytes
NPSWF32_6! native_ShockwaveFlash_TCallFrame + -41978 bytes
NPSWF32_6! native_ShockwaveFlash_TCallFrame + -42229 bytes
....
I got
len == unsigned long 402
const char *buf ==:
03CE2110  43 6F 6E 74 65 6E 74 2D 74 79 70 65 3A 20 61 70 70 6C 69 63 61 74 69 
6F 6E 2F 78 2D 6D 6D 
2D 66  Content-type: application/x-mm-f
03CE2130  6D 66 0A 43 6F 6E 74 65 6E 74 2D 6C 65 6E 67 74 68 3A 20 33 34 34 0A 
0A 00 00 00 00 00 02 
00 30  mf.Content-length: 344.........0
which is incomplete header and NS_NewPluginPostDataStream() fails on 
    157       if (!(crlf = PL_strnstr(data, "\r\n\n", contentLength))) {
    158         nsMemory::Free(newBuf);
    159         return NS_ERROR_NULL_POINTER;
    160       }
...
there is no "\x0Hello World" in there, 
did I miss something, what is the content?

Comment 17

[Peter Lubczynski]

Should we be looking for \n\n as the marker for the end of the header instead of
\r\n\n?

See bug 60228 for the orrignal source of that code.

Comment 18

[av (gone)]

Serge has a hand on it. Reassigning, please feel free to bounce it back to me.

Comment 19

[serge (gone)]

according to the 4.x spec 
http://developer.netscape.com/docs/manuals/communicator/plugin/pgfn2.htm#1007763
>NPN_PostURLNotify supports specifying headers when posting a memory buffer. 
below line #1007754
>For protocols in which the headers must be distinguished from the body, such as 
>HTTP, the buffer or file should
>contain the headers, followed by a blank line, then the body. If no custom 
>headers are required, simply add a blank
>line ('\n') to the beginning of the file or buffer. 
So do we have to check for single '\n', "\n\n" or "\r\n\n"?

Comment 20

[av (gone)]

From HTTP1.1 spec:

        generic-message = start-line
                          *(message-header CRLF)
                          CRLF
                          [ message-body ]
        start-line      = Request-Line | Status-Line

So, looks like each header should be terminated with CRLF. In addition, one more 
CRLF should be added to the end of the last header.

So, I see it like this: 

1. if we see '\n' and some char following, that means there is no header. We 
should replace it with "\r\n"

2. if we see "\r\n\n" or "\n\n" this probably means somebody wrongfully 
indicated the end of the headers, and we should fix it with "\r\n\r\n" again

3. I think we also should fix all previous occurences of a single '\n' between 
headers

Serge, can you try this and if it fixes the problem?

Comment 21

[av (gone)]

...and SEE if it fixes the problem?

I can imaging why this is happenning. The Plugin doc says "\n" so people do just 
that. The HTTP spec says "\r\n" and Necko probably expects it exactly in this 
form. Does it make sense to anybody?

Comment 22

[Darin Fisher]

it's good to use \r\n, but traditionaly \n is okay too.  mixing them can be
problematic and probably confuses more than just a couple servers out there.

Comment 23

[Peter Lubczynski]

Wow, we really need to do a lot of fix up on the raw headers before passing them
to Necko. I wonder if something else can already do this for us, Darin?

If not, |NS_NewPluginPostDataStream| will need to be smart enough fix up the
stream like Andrei says in comment #20. The NPAPI spec says one thing and the
HTTP sepc says another and we need to be prepared for all kinds of input.

Comment 24

[serge (gone)]

Attachment: patch v1

Comment 25

[av (gone)]

Serge, would you please explain what the patch does? This will be useful for 
those who just wants to know what we are doing not going into implementation 
details.

Comment 26

[serge (gone)]

Well, I've just slightly changed peter's implementation (attachment 62115 [details] [diff] [review] for 
bug #108966) of NS_NewPluginPostDataStream(),
it does handle "\n\n" as end of http headers and first single '\n' as buffer w/o 
headers.
Peter, could you take a look on the patch, please?

Comment 27

[Peter Lubczynski]

Comment on attachment 63929 [details] [diff] [review]
patch v1

r=peterl

Comment 28

[Darin Fisher]

peter: AFAIK there aren't any utility functions in the code base that you can
use to perform this fixup.

Comment 29

[Darin Fisher]

Comment on attachment 63929 [details] [diff] [review]
patch v1

sr=darin

but, this function should really be added as a method on some service.
NS_NewPluginPostDataStream should really be a lightweight wrapper function
instead.

Comment 30

[Ari Berger]

We are having a related plugin with our viewpoint windowless plugin.  I am not 
sure if it should be a different bug.
I post using NPN_PostURLNotify.  I applied the patch mentioned in this bug.  I 
am posting data- just some text (eg abcdefg).  I prepend it with \n, so the 
data I put in is "\nabcdefg".  The data gets posted to the server, which 
responds with a 400.  I look using a TCP peek program, and the problem seems to 
be that NONE of the http headers are there.  There is no POST, no http 1.1, no 
url.
Is there anything obvious that comes to mind? I will put together a test case 
ASAP.
I am working on the .9.4 branch.

Comment 31

[serge (gone)]

Ari, you are right, without Content-length: header POST request is not going to 
work. We have to add that header if there is no one, that is easy,
but what about Content-type: do we have to add  it as "text/html" if it's 
missing? 
Darin what do you think?
Thanks.

Comment 32

[av (gone)]

I had an impression that Necko should care of this if the |postHeaders| is null.

Comment 33

[Darin Fisher]

it's important that the content-length be specified.  otherwise, the server
won't know the length of the http request and therefore won't know when the
request has been completely sent.

the content-type header on the other hand is probably not so essential.  does
the plugin API even provide a way for the plugin to specify the content-type?  i
think it's probably safe to leave out a content-type header if the plugin fails
to provide one in the plugin data stream.

Comment 34

[av (gone)]

The code eventualy calls 
nsILinkHandler::OnLinkClick(..., postDataStream, postHeadersStream)

Here is my vision of how it works.

Before it comes to it a plugin can tell us that it has a header in a separate 
buffer (e.g. Java plugin). In this case we create two streams: for the raw data 
and the headers and pass them to the link handler.

Legacy plugins however do not have this kind of access, well they actualy do, 
but they cannot do this via NPAPI, so they give us a buffer which may or may not 
content headers. In the code which is being fixed in this bug we try to find the 
headers and fix them if they are not up to the spec. Whether there are headers 
or not we now create only one stream -- |postDataStream|, which will include 
headers in itself and pass |null| in place of |postHeadersStream|

If all this is correct, it does not look right to me. I would expect 
|OnLinkClick| to create headers for me if the |postHeadersData| is |null|. This 
is OK if the data from the plugin is just a raw data but we don't want more 
headers if the data already contain them.

Looks like the logic in |nsPluginInstanceOwner::PostURL| should be rewritten. We 
shuold probably separate the headers (if found) from the data and pass them 
separately to the link handler. If not found, the link handler will create some 
default headers for us (should it?).

Darin, does it make sense?

Comment 35

[av (gone)]

The last two paragraphs in my last comment are probably not quite accurate. The 
standard header should be added by Necko in any case. Any additional headers the 
plugin wants to add can be done either as a separate buffer or as a part of the 
data buffer. We just need to make sure all CR and LF chars are in appropriate 
places.

Comment 36

[serge (gone)]

That is right, I'm writing data buffer parser,
which  should be smart enough to figure out end of headers, if header separator 
is LF change it to CRLF, because our standard headers use it,
and if there is no content-length header add it to the stream.
Definitely this parser should be implemented as a method of some service,
the question is which one, av, peterl any suggestions?
BTW: in current implementation every thing work fine when content-length header 
is present, headers separator is CRLF and end of headers is CRLFCRLF.

Comment 37

[Arun Ranganathan]

As folks like chofmann and others are looking to plan some embedding stuff off
of the branch, they need eta data.

Comment 38

[serge (gone)]

I'm going to post new patch probably tomorrow, so ETA: 01/14/02

Comment 39

[serge (gone)]

I run into some discrepancy for POST url as file, I have not figure out how we 
add content len header and CRLFCRLF as end of http headers to the post data 
stream. Does it work before?
Investigating... 

Comment 40

[Peter Lubczynski]

Files go down a different code path, through NS_NewPostDataStream which creates
an instance of the LocalFileInputStream component so it may be harder to do the
fixup, if needed?

Comment 41

[serge (gone)]

Attachment: patch v2, it does add content-len header to post buffer & substs single LF with CELF in headers & deals with post url as file

av, peter would you please take a look on this, thanks.

Comment 42

[av (gone)]

Darin, who should be responsible for adding content-length header? I think we 
should be consistent on this. I talked to gagan and he said that there was a 
discussion that Necko side should evaluate the length of the data and take care 
of the header. What is the latest?

Another question. |nsILinkHandler::OnLinkClick| takes post data stream and 
headers stream. Post data may or may not contain headers, headers stream may or 
may not be present at all. Are all of those cases accounted for?

Comment 43

[Darin Fisher]

take a look at the implementation in nsHttpChannel::SetUploadStream.

necko doesn't know if the given nsIInputStream contains headers or not, so if in
the call to:

SetUploadStream(in nsIInputStream, in string contentType, in long contentLength)

contentType is null, necko will assume that the input stream contains all
necessary headers as well as the required header/body line break.

if contentType is not-null, then necko will assume that the input stream
contains only the message body (w/o any headers).  then necko will use the
specified contentType and contentLength to set appropriate request headers.

Comment 44

[av (gone)]

> contentType is null, necko will assume that the input stream contains all
> necessary headers

By 'all' -- do you mean ALL, even standard headers?

Comment 45

[Darin Fisher]

i mean that the stream must contain at least Content-Type and Content-Length
headers.  the usual request headers will be automatically sent first.

Comment 46

[serge (gone)]

well, the patches for bugs 112479 119625 (with implementation Darin described)
are not on the 0.9.4 branch, we can't use it in there anyway.

Comment 47

[serge (gone)]

Attachment: the same as v2 only uses nsAutoVoidArray instead fixed size array to count single LF in headers

Comment 48

[serge (gone)]

Attachment: patch v3; fixed open file with native path, added diff nsPluginViewer.cpp

Comment 49

[Judson Valeski]

can we get r/sr on this?

Comment 50

[Brian Nesse (gone)]

In nsPluginHostImpl::PostURL() it appears that the incoming argument |postData|
is replaced with either |tmpFileName| or |newDataToPost| at the bottom, if
|isFile| is true, this is freed. This seems prone to a) leaking the original
contents of  |postData| and b) returning a deleted pointer... both concern me.

In ParsePostBufferToFixHeaders()
+  const char *pEod = inPostData + inPostDataLen;
+    while (s <= pEod) {
Is the buffer 0 terminated (i.e. 1 byte longer than inPostDataLen?) if not, you
could read past the end of the buffer here...

+      if (!pSCntlh && 
+          (*s == 'C' || *s == 'c') && 
+          (s + sizeof(ContentLenHeader) - 1 <= pEod) &&
[snipped]
+        s += sizeof(ContentLenHeader);
This seems dangerous also, if size - 1 == pEod s + size is past the buffer end.

In CreateTmpFileToPost()
+  nsCOMPtr<nsILocalFile> file = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
+  if (NS_FAILED(rv) || 
+    (NS_FAILED(rv = file->SetURL(postDataURL)) && NS_FAILED(rv =
file->InitWithPath(postDataURL))) ||
This doesn't compile on the Mac... I don't think SetURL is a member of
nsILocalFile...

Comment 51

[serge (gone)]

Thank you Brain for quick review.
>This seems prone to a) leaking the original
>contents of  |postData| and b) returning a deleted pointer... both concern me
I do free only memory I've alloced in rv = CreateTmpFileToPost(postData, 
&tmpFileName) call
original contents of  |postData| is (const char*) and we cannot modify it.

>+    while (s <= pEod) {
You are right, probably I wa plaing with '\n' case at start of buffer, or 
something..:(
I'll chahge all <= pEod to < pEod
>+        s += sizeof(ContentLenHeader);
it's going to be +        s += sizeof(ContentLenHeader) -1;

>This doesn't compile on the Mac... I don't think SetURL is a member of 
nsILocalFile
the patch (attachment 64164 [details] [diff] [review]) for bug 100212 remove "URL" attribute from 
nsIFile.idl
I'm going to change it for the trunk, but it should be still valid in 0.9.4 
branch

Comment 52

[Brian Nesse (gone)]

>I do free only memory I've alloced in rv = CreateTmpFileToPost(postData, 
>&tmpFileName) call original contents of  |postData| is (const char*) and we
>cannot modify it.
This is true, but the memory allocated by this call (in the !isFile case)
+    ParsePostBufferToFixHeaders(postData, postDataLen, &newDataToPost,
&newDataToPostLen);
is not freed because:
+      if (isFile && postData) {
+        nsCRT::free((char*)postData);
+      }

It seems to me that the code would more readable if you simply got the new name
into a common local variable in both cases instead of trying to reuse the input
argument.

Comment 53

[serge (gone)]

we are using nsIStringInputStream::adoptData(char *data, int length)
to set the stream, on destroy stream calls nsMemory::Freei() for data.
>It seems to me that the code would more readable if you simply got the new 
>name...
I agree.

Comment 54

[Brian Nesse (gone)]

Ok, I think I see it... that is absolutely screaming for some comments to
explain why we appear to be not releasing memory, etc. Thanks for clearing it up
for me.

Comment 55

[serge (gone)]

Attachment: v4 for 01-15-02 trunk, which addresses all Brain's comments

Comment 56

[Brian Nesse (gone)]

Comment on attachment 65125 [details] [diff] [review]
v4 for 01-15-02 trunk, which addresses all Brain's comments

+  PRInt64 fileSize;
[snip]
+  if (fileSize != 0) {
You should use the LL_ macros (i.e. LL_IS_ZERO(fileSize)) when using 64 bit
ints to insure no XP issues.

With that fix. r=bnesse.

Comment 57

[serge (gone)]

>You should use the LL_ macros (i.e. LL_IS_ZERO(fileSize))
will do so, thanks Brian.

Comment 58

[av (gone)]

Three minor things after I looked at the patch briefly (sorry, had absolutely no 
chance to do it earlier):

1. |newDataToPost| should be primed with null. The parser function can be 
rewritten later and somebody may forget to assign null there.

2. instead of doing 

    if (!something)
      return NS_ERROR_NULL_POINTER;

we should go

    NS_ENSURE_ARG_POINTER(something);

3.    if (isFile && tmpFileName) {
        nsCRT::free(tmpFileName);

why to check |isFile|?

All those maybe fine for the branch check in but would be nice to fix on the 
trunk.

Comment 59

[Darin Fisher]

ok, so i looked over the patch... it's rough, but seems accurate to me.

for example, the uses of sprintf should be replaced with PR_snprintf... please
never ever use sprintf.  it makes code very fragile.

also there is an indentation problem in the PostURL() method.

at any rate, sr=darin

Comment 60

[serge (gone)]

on the 0.9.4 branch:
Checking in modules/plugin/base/src/nsPluginHostImpl.cpp;
new revision: 1.288.2.16; previous revision: 1.288.2.15
--
Checking in modules/plugin/base/src/nsPluginHostImpl.h;
new revision: 1.55.14.6; previous revision: 1.55.14.5
--
Checking in modules/plugin/base/src/nsIPluginHost.h;
new revision: 1.20.116.2; previous revision: 1.20.116.1
--
Checking in modules/plugin/base/src/nsPluginViewer.cpp;
new revision: 1.74.2.7; previous revision: 1.74.2.6
--
Checking in modules/plugin/base/public/nsPIPluginHost.h;
new revision: 1.1.64.1; previous revision: 1.1
--
Checking in layout/html/base/src/nsObjectFrame.cpp;
new revision: 1.248.2.23; previous revision: 1.248.2.22

Comment 61

[Ari Berger]

Well, I don't post binary data, so I can't verify that part.  But I have 
verified that I no longer need to prepend the content-length header. 
Works for me.

Comment 62

[shrirang khanzode]

Thx for the help, Ari ! 

Comment 63

[serge (gone)]

Attachment: v5 the same as v4, with PR_snprintf, and manor changes in nsPluginHostImpl::ParsePostBufferToFixHeaders()

Comment 64

[bmartin]

It appears that this may be working as expected with CS/Talon 112a 

Launch CS/Talon 
Go to http://lxr.mozilla.org/mozilla/source/modules/plugin/tools/tester 
Select "Click here to launch the tester" 

I set the following parameters: 
API call = NPN_PostURL 
URL = http://www.mozilla.gr.jp:4321 
Target = _npapi_Display (default) 
buf = "Hello World" 
file = FALSE (default) 
Select GO button 

Results: 
len = 11 (sets itself automatically) 

Top frame contains: 
HTTP Client Net Debugger 
Server: Tokyo.Mozilla.Gr.JP/210.160.137.138, Port: 4321 
Client: x98A3A5C2.pix.aol.com/152.163.165.194, Port: 3138 
---------------- 
HTTP Request  & Body 

POST / HTTP/1.1 
Host: www.mozilla.gr.jp:4321 
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.4.2) Gecko/20020120 CS 
2000 7.0/7.0 
Accept: text/xml, application/xml, application/xhtml+xml, text/html;q=0.9, 
image/png, image/jpeg, image/gif;q=0.2, text/plain;q=0.8,
text/css, */*;q=0.1 
Accept-Language: en-us 
Accept-Encoding: gzip, deflate, compress;q=0.9 
Keep-Alive: 300 
Connection: keep-alive 
Referer: 
http://lxr.mozilla.org/mozilla/source/modules/plugin/tools/tester/testcase/teste
r.html 
Content-length: 11 

Hello World 
----------------------------------------------- 
HTTP Response (Content-Length omitted) 
======================================================================== 
The Lower Frame Contains: 
NPN_PostURL(0x01e87eb4, 0x0065f418("http://www.mozilla.gr.jp:4321/"), 
0x0065f398("_npapi_Display"), 11, 0x0065f318("Hello
World"), FALSE) 
Returns: NPERR_NO_ERROR 
Time: 91243 91243 (0) 

Comment 65

[bmartin]

Attachment: results comparison CS/Talon vs N6094 vs N4.78

Comment 66

[Brian Nesse (gone)]

Comment on attachment 66065 [details] [diff] [review]
v5 the same as v4, with PR_snprintf, and manor changes in nsPluginHostImpl::ParsePostBufferToFixHeaders()

Additional changes appear ok. r=bnesse.

Comment 67

[Darin Fisher]

Comment on attachment 66065 [details] [diff] [review]
v5 the same as v4, with PR_snprintf, and manor changes in nsPluginHostImpl::ParsePostBufferToFixHeaders()

sr=darin

Comment 68

[serge (gone)]

Thanks all.
Checked into the trunk.
Nominating for 0.9.8 branch.

Comment 69

[serge (gone)]

resolved as fixed

Comment 70

[Asa Dotzler [:asa]]

this did not land for 0.9.8 so anyone working with the 0.9.8 branch who needs
this will want to patch their tree. 

Comment 71

[shrirang khanzode]

.