33193 - libxpt needs sanity check for possible typelib truncation

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P3)

Description

[John Morrison]

From bug #33183, where browser.xpt was truncated, resulting in a quick, 
fatal crash of the browser on launch with an error message:

 "FATAL: can't no room for 1 in cursor." ...

  ---- Additional Comments From [e-mail]  2000-03-24 10:22 -------
  We should probably do some sanity checking to make sure that the file length
  matches TypeLibHeader.file_length, so we can report a better error message.
  
  I'll take a bug on that, but it won't get fixed before the party.
  ----

So, this would be that bug.

Comment 1

[Mike Shaver (:shaver emeritus)]

jband begged.  No, really!

Comment 2

[John Bandhauer]

Changed the subject to reflect that the specific check we're talking about is 
truncation of the typelib file. Passing in the file length to XPT_DoHeader for 
verification when doing XPT_DECODE ought to do the trick.

Comment 3

[Gervase Markham [:gerv]]

This bug has not been touched for more than nine months. In most cases, that 
means it has "slipped through the net". Please could the owner take a moment to 
add a comment to the bug with current status, and/or close it.

Thank you :-)

Gerv

Comment 4

[John Bandhauer]

mass reassign of xpconnect bugs to [e-mail]

Comment 5

[David Bradley]

From what I can tell the size entry in the header is not being set. So 
additional logic needs to be added to set this value.

Comment 6

[John Bandhauer]

(If I understand you) I think it goes the other way. Once we are filling in the 
header it is too late. As I said above, I think the XPT_DoHeader should get 
a new param used in the decode case where the caller would pass in the size of 
the file it read. XPT_DoHeader would take that into account. If that size were 
smaller than sizeof(XPTHeader) it would *know* it was bogus. Otherwise, it would 
xdr up to getting header->file_length and fail there if the sizes don't match.

Comment 7

[David Bradley]

Right, you need a check before the attempt to read the header. It's the 
subsequent check of file_length after you read the header that I was refering to 
that has a problem. file_length in XPTHeader is always zero.

Comment 8

[John Bandhauer]

Right, sorry for being dense. I didn't realize that we weren't evven writing the 
length into the file. so we're not conforming to the typelib spec in this 
regard. Hmm, that doesn't look so easy to fix. Perhaps that deserves its own 
bug. Further, it may just not be worth the effort to finx anytime soon.

Comment 9

[David Bradley]

I had already fixed the length bug and had started on adding the logic to check. 
So I figured I'd go ahead and complete it.

There were three conditions I tested for.

1. Zero length XPT file.
2. XPT file with a length less than the header
3. XPT file with a length more than the header but less than the stated size in 
the header.

I moved the code that writes the initial part of the header into a separate 
function. This allows me to call it from within the XPT_DoHeader to do the 
normal stuff and outside of the function to rewrite the header part with the 
size. This, admittedly, is not an optimal solution, but it has the least impact 
to the overall existing structures and interface. I then fixed up xpt_link.c, 
and xpt_typelib.c to rewrite the header with the proper size.

Comment 10

[David Bradley]

Attachment: Fixes length not getting set and adds length check

Comment 11

[John Bandhauer]

David: It would be good if you update this bug with just the parts that are not 
in the patch to bug 83501 (not to rush you, of course!).

Also, (in case you did not think of it), you are going to have to add special 
case logic to 'trust' xpt files that do not have any filelength info. We don't 
want to break compatibility with deployed xpt files if we don't have to.

Comment 12

[David Bradley]

Yes I was going to do that. The problem I ran into is how to create this patch.
If I diff against the tip it won't be correct when it's applied after the patch
to 83591. I figured the patches would invalidate themselves since they both
involve changes in the same area. I didn't know of a way to diff just local
files and get the same format as cvs diff. So I was figuring on checking in the
patch for 83591 and then diffing. I did some playing and diff does seem to
generate a format that patch will use, although it's not identical to 83591.
I'll attach that.

Also, does this patch really need to wait? It still is a bit more risky than
83591, but with the allowance of xpt files with zero length, it's not as risk as
originally written.

Comment 13

[David Bradley]

Attachment: This patch contains just the logic to test the file length in the header

Comment 14

[David Bradley]

Attachment: The real patch with correct logic

Comment 15

[John Bandhauer]

Are you still waiting for review on this one?
Have you been running with it in your build?
I wonder if it shouldn't check '!=' rather than '<'. I suppose it is *far* less 
likely to see a file with garbage inserted in the middle than simply a truncated 
file. So, that would make little difference.

Comment 16

[David Bradley]

Attachment: Fresher patch, added better error message

Comment 17

[David Bradley]

I used the < rather than != because the value tested is the memory pool size.
Ideally and in practice this is the size reported in header, but given the
concept I didn't feel safe using !=. Maybe I'm too paranoid.

Comment 18

[David Bradley]

Attachment: Fixed mispelling, and add info to error message

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 55720 [details] [diff] [review]
Fixed mispelling, and add info to error message

r=jst

Comment 20

[John Bandhauer]

Comment on attachment 55720 [details] [diff] [review]
Fixed mispelling, and add info to error message

sr=jband

Comment 21

[David Bradley]

Forgot to mark fixed

Comment 22

[David Bradley]

Arg! wrong bug

Comment 23

[David Bradley]

patch checked in

Comment 24

[Phil Schwartau]

Marking Verified -