158500 - Binary data in multipart/x-mixed-replace get an extra line end when saved or viewed.

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Jesse Houwing]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1b) Gecko/20020717
BuildID:    2002071708

I've made a piece of coldfusion code that makes it possible to offer multiple
files for download with only one HTTP request.

Text seems to work fine (it doesn't mind an extra line end). But for images and
executables it poses a problem.

I'll attach the exact output shortly.

Comment 1

[Jesse Houwing]

Attachment: Exact output multipart http file.

This is the output.

The line-end at the end before the final multipart boundary should be there,
but should NOT be part of the binary data.

Comment 2

[Jesse Houwing]

My test site isn't always on btw.

Comment 3

[Jesse Houwing]

when savinf the file btw, by setting the content type to
application/octet-stream, the resulting file can still not be read by mozilla,
but Internet explorer and Photoshop have no trouble at all.

Comment 4

[Jesse Houwing]

hitting back, and then forward does show the image.

I'm sure that is a bug too, but...

Comment 5

[Jesse Houwing]

Just tested with 1.1b under XP, and it works there.

Comment 6

[Jesse Houwing]

And I tested it at work under 1.1a (2002061404/NT4) and it works there too, but
the latest nightly does not.

Comment 7

[Darin Fisher]

*** Bug 159366 has been marked as a duplicate of this bug. ***

Comment 8

[Darin Fisher]

-> me

Comment 9

[Jean-Marc Desperrier]

The dependant bug 159366 is for me a good nsenterprise candidate, it is a
serious concern for us in a deployment for a major french credit insurance
company and I'd like it to be fixed in Netscape 7.

As there is no Priority and no Target Milestone for this bug, it seems to me
nobody is actively working on it.
Is nobody available for that ?

Comment 10

[Darin Fisher]

Attachment: v1 patch

simple patch... we should be stripping LF (or CRLF) preceding each token.  this
is what 4.x appears to do, and it seems to be what's required by RFC 1341
section 7.2.1 (http://www.w3.org/Protocols/rfc1341/7_2_Multipart.html).

Comment 11

[Jean-Marc Desperrier]

Yes, I forgot in my argumentation that I expected this to be a low impact, low
risk patch.

In the comments for bug 159366, I also quoted RFC 2046 (update to 1341)
explicitly saying that this is the desired behaviour.

Thanks for the fast patch Darin, I hope it can make in releases even before 1.2

Comment 12

[Doug Turner (:dougt)]

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

do you really want to have this function inline?  

r=dougt

Comment 13

[Darin Fisher]

dougt: no, it doesn't have to be inline, but i figured since it's a small
function without any loops, that inlining makes sense.  perhaps i should just
leave it static and let the compiler decide what's best.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky if you change |len--| to |--len|.

Comment 15

[Darin Fisher]

fixed-on-trunk

Comment 16

[Darin Fisher]

*** Bug 159366 has been marked as a duplicate of this bug. ***

Comment 17

[Darin Fisher]

Jean-Marc: i'm not sure that this will be considered for mozilla1.0.1 since this
problem only applies to multipart/mixed, which isn't even supported by MSIE. 
but, i'll let project management decide.

Comment 18

[Jean-Marc Desperrier]

My concern is not in fact Mozilla 1.0.1 , but NS 7 final release, because
enterprise users will probably want to use it over Mozilla.

That would be disappointing if NS 7 ships without the patch and I suspect it's
already a bit late, but I'm raising the issue.
Will test the fix when the new nightly is available, should be in a few hours.

Yes, the page where we have the problem is Netscape specific, the IE page uses
VB script, and ActiveX I believe.

Comment 19

[Syd Logan]

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

what if cursor > token? Using unsigned arithmetic, you could potentially get a
large 32 bit number in that case and go reading memory not belonging to the
token. If we are going to take a patch like this, it might be good to add a
check for cursor >= token at the start and return 0 if so.

Comment 20

[Darin Fisher]

syd: if what you say could happen then we are no better off without this patch.
 notice that the second parameter to SendData is a PRUint32 and without my patch
we were simply calling SendData(cursor, token - cursor).  clearly, if token <
cursor, then we're screwed anyways.

Comment 21

[Paul Wyskoczka]

Let's fix this in Buffy.  -adt

Comment 22

[Jean-Marc Desperrier]

Can someone consider this bug for inclusion in 1.0.2 ?

Comment 23

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

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

a=[e-mail] for 1.0 branch; change mozilla1.0.2+ to fixed1.0.2 when
checked in

Comment 24

[Darin Fisher]

fixed1.0.2

Comment 25

[Jaime Rodriguez, Jr.]

benc: can you pls verify this as fixed on the 1.0 branch, then replace
"fixed1.0.2" with "verified1.0.2"? thanks!

Comment 26

[benc]

What are the STEPS for this bug?

Viewing the link, or saving it?
Mozilla 1.1: viewing results in:
The image, then a text message:
"The image “http://toaom.student.utwente.nl/test.cfm” cannot be displayed,
because it contains errors."

Communicator displays some HTML source before loading the image.

Comment 27

[Jaime Rodriguez, Jr.]

[e-mail] or darin -- can you pls answers ben's question, so
that he can verify this issue as fixed? thanks!

Comment 28

[Jesse Houwing]

Just tested in:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2b) Gecko/20020924

It should show the picture. if it doesn't and gives an error, then ir's not
working the way it is supposed to.

Comment 29

[Jesse Houwing]

It should show some HTML, then some Soure, then the image. Depending on the
speed of your internet connection it could just show an image. If you are
running unpatched it should end with an error saying the image contains an error.

Comment 30

[Jean-Marc Desperrier]

Benjamin, the result you get is the normal result in a fail case.
As 1.1 does not include the patch, this is not surprising. 
Only version from 1.2a, and the 1.0.2 candidates have the patch.

Fail case  :
Some HTML, then some source, shows partially loaded image, then after the image
is fully loaded, sees the unwanted end of line and then complains.

Succes case :
Some HTML, then some source, shows partially loaded image, then stops
downloading and show fully loaded image without problems.

Comment 31

[benc]

This didn't work in Win32 or Linux branch builds.

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0.2) Gecko/20020924

That was too close to the checkin, so I updated my Linux build to 20021009, and
it is giving the error. 

I don't know how to reopen for the branch, so I'm reopening the whole bug.

I'm going to move to the trunk now.

Comment 32

[Gayatri Rimola]

Removed the fixed1.0.2 keyword to "re open" the bug on the branch.

Comment 33

[benc]

REOPEN:

Also doesn't work in;

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.2b) Gecko/20021009

I am not seeing much of anything, then the error. No code, no image.

Comment 34

[benc]

Darin talked to me, it looks like some cache settings from an FTP test affect this.

I'm going to retest, results should be in by tomorrow.

Comment 35

[benc]

moving back to resolved fixed.

Comment 36

[benc]

VERIFIED/fixed - trunk, mozilla 1.2a all plats.

Comment 37

[benc]

VERIFIED102
allplats 20020924

sorry for the confusion. I'm going to go find that FTP bug that suggested I
should test w/ zero caches, and invalidate it now...

Comment 38

[Jesse Houwing]

Just tested in 2002100808 1.2+ and it still works fro me.