84332 - Crash in Byte Range Requests for this PDF File

Status: VERIFIED FIXED

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

Description

[lmcquarr]

From Bugzilla Helper:
User-Agent: Mozilla/4.77 [en] (Windows NT 5.0; U)
BuildID:    2001060604

A byte range request on this particular PDF file causes a crash. 


Reproducible: Always
Steps to Reproduce:
1.Install Adobe Acrobat Reader (www.adobe.com), version 5.0.
2.Install mozilla and drop the nppdf32.dll plug-in into the mozilla plugins dir. 
3.Run Acrobat and turn off background downloading (Edit menu, Prefs, General 
Prefs, Options, check box disables background downloading).
4. Go to the above URL.  
5. After the first page draws, using the scroll bar, try to scroll
to page 2.
6. Crash in gkplugin.dll.


Actual Results:  Crash.

Expected Results:  See page 2 of the doc draw in acrobat. 

I have been testing the new byte range request support.  It looks
great so far, except for this crash!

Comment 1

[Doug Turner (:dougt)]

can you get a stack crawl?  do you have any idea where it is crashing?

Comment 2

[av (gone)]

I don't see the crash with the debug build but I do see the assertion 
on corrupted data which most likely will cause crash in released builds:

NTDLL! 77f7629c()
NTDLL! 77f8322e()
KERNEL32! 77f12e09()
_CrtIsValidHeapPointer(const void * 0x02c121b0) line 1637 + 21 bytes
_free_dbg_lk(void * 0x02c121b0, int 1) line 1044 + 9 bytes
_free_dbg(void * 0x02c121b0, int 1) line 1001 + 13 bytes
operator delete(void * 0x02c121b0) line 49 + 16 bytes
nsPluginStreamInfo::RequestRead(nsPluginStreamInfo * const 0x0417ce90, 
nsByteRange * 0x0012f7ec) line 1149 + 15 bytes
_requestread(_NPStream * 0x0417b168, _NPByteRange * 0x0012f7ec) line 1084
NPPDF32! 04b74a53()
NPPDF32! 04b73150()
NPPDF32! 04b74f2f()
0417c940()

Comment 3

[Doug Turner (:dougt)]

av, how big is the byte range request coming in?  If it is over 1024, we will
corrupt data.  that should be fixed.

Comment 4

[Peter Lubczynski]

The leak needs to be fixed as well. Bug 83183. Welcome back av!

Comment 5

[av (gone)]

Doug, the request consists of 85 ranges and yes, the very first one is 1044 
bytes long.

Comment 6

[av (gone)]

If this is all it takes the fix should be relatively easy. Maybe even for 9.1?

Comment 7

[Doug Turner (:dougt)]

okay. 0.9.1?  Lets see if anyone cares to get this in for 0.9.1

Comment 8

[Arun Ranganathan]

In an e-mail exchange between Liz McQuarrie and myself this is what she said:

"You probably can [release 0.9.1] without a fix for this, since the basic
functionality works,
and reasonably well. It will be, however,  very important to figure out what is
going on before
you ship [0.9.2]. I have been stress testing the byte range requests and am getting
period crashes that are less reproduceable.  I am hoping that whatever
is causing the crash in the bug is behind my other crashes (I am working on
making more reproduceable test cases for that)."

Based on this information, it seems to make sense to remove this from the radar,
but we're still on the hook to fix this by 0.9.2.
 

Comment 9

[Marek Z. Jeziorek]

moving to 0.9.2

Comment 10

[av (gone)]

Attachment: patch to fix hacky string

Comment 11

[av (gone)]

Doug, this patch fixes the hardcoded string size, but Acrobat still reports 
errors in the incoming stream. Could you please apply it and see what can 
possibly be wrong in the network layer?

Comment 12

[Doug Turner (:dougt)]

The network layer has not changed.

Comment 13

[lmcquarr]

I applied the patch and the problem with page 2 goes away.  But if I go
to page three of the same document (with background downloading turned off),
I get a page error in acrobat.  This implies to me that some data
returned to Acrobat by mozilla is corrupt.  I am examining that right now.

Comment 14

[lmcquarr]

After applying the patch, 
I am definitely seeing cases where the bytes that are delivered to 
Acrobat are slightly different that the real bytes.  I am writing
a tool to take the bytes that are delived to us and dump them out to
a file in the correct position, so that I can more easily figure out
where we got messed up bytes.  

These kind of problems are very tricky to resolve.  When someone is ready to
work on this, I volunteer to come by with my code and tools, if necessary,
to find the problem.  Or maybe I will have a tool that I can provide
you that might be enough. 

If you have background downloading disabled, the wildtype.pdf file above
will produce a page drawing error on page 2 or page 3  (it gets screwed
up on one or the other each time).  The page drawing error means that
we got some bad bytes in our page content stream -- since it is compressed,
it is hard to pinpoint exactly where the bad bytes are without the tool
I am writing. If you go to page 2, and get a blank screen, go to page
3 and then go back to page 2 and you will get the page drawing error.

Comment 15

[Doug Turner (:dougt)]

Hmm.  Moz will usually give you more than you request, but we shouldn't be off.  
cc' rpotts, the last person to touch the multipart converter.

Comment 16

[av (gone)]

Adding 'fix in hand' for a crasher part. Can anybody please review the patch. I 
think dynamic string should go in anyway. Probably we need to file a separate 
bug against the remaining issue.

Comment 17

[rpotts (gone)]

hey Andrei,
here are a couple of comments on the patch:

1. nsCStrings string, firstbyte and lastbyte should probably be an 
nsCAutoStrings to avoid a automatic heap allocation/deallocations...  Especially 
firstbyte and lastbyte - since they are being created and destroyed within the 
for loop.

In fact, after looking at the code closer, it looks like 'firstbyte' and 
'lastbyte' can be removed completely!  why not dothe following:
    string.AppendInt(range->offset);
    string.Append("-");
    string.AppendInt(range->offset + range->length-1);

This would eliminate the need to create the intermediate CStrings...

2. rather than copying 'string' into 'outstring' and removing the possible 
trailing comma... I believe that you can do the following:
    string.Trim(",", PR_FALSE);
which will remove any trailing commas directly from the CString...  This would 
eliminate the need for 'outstring'

3. Then you could do the copy directly into *rangeRequest which would be 
clearer. ie.
    *rangeRequest = string.ToNewCString();

4. This would also allow you to use the standard deallocator to free 
rangeString. ie.
    nsMemory::Free(rangeString)

How do these comments sound?  I realize that they go beyond the scope of the 
changes that you made :-) But, I think that they make the code cleaner and 
clearer to understand...

-- rick

Comment 18

[lmcquarr]

Bug 85529 is the new bug I entered for the bad bytes that mozilla is returning.

Comment 19

[av (gone)]

Rick, thanks for the comments! I will indeed take them into account and post new 
patch soon.

Comment 20

[av (gone)]

Liz, thank you for all your help. Now as we have the bug for wrong bytes filed, 
do you think we can close this one for a crash? If so, we should probably 
nominate 85529 as an rtm stopper.

Comment 21

[av (gone)]

Attachment: patch -- second try, incorporating rpotts' comments

Comment 22

[lmcquarr]

Yes -- this can be closed now that a new bug has been filed for bad bytes
(85529). 

Comment 23

[rpotts (gone)]

looks good to me.  sr=rpotts

Comment 24

[Peter Lubczynski]

r=peterl

Comment 25

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for the trunk

Comment 26

[av (gone)]

Checked in. Marking fixed.

Comment 27

[shrirang khanzode]

yeah, verified on win 2001061905 trunk on windows that this doe snot crash 
anymore. However pages 2 and 3 do not laod and I see the error Liz mentioned. 
Bug 85529 cover that. VERIFIED