1190639 - Callers of nsIObjectInputStream methods should not use the result of failed Read*() methods

Status: NEW

(Core :: XUL, defect)

Description

[Andrew McCreight [:mccr8]]

There are a bunch of Read*() calls in nsXULPrototypeElement::Deserialize(). The code saves the return value, and eventually fails if each read fails, but that means in the interim you are using whatever the old value is, which seems questionable. The patches in bug 1188234 should be enough to ensure we don't crash even if we randomly set the values, but hopefully this code is not perf sensitive enough that can't just immediately return on failure. I split this out from bug 1188234 because it seems a little riskier.

Comment 1

[Andrew McCreight [:mccr8]]

I've gone ahead and audited all of the callers of the Read methods on nsIObjectInputStream and I have some patches to fix a few places that don't properly handle failure. I looked at Read8, Read16, Read32, Read64,  ReadFloat (never called from C++), ReadDouble (never called from C++), ReadCString, ReadString, ReadBytes, ReadByteArray (never called from C++), ReadArrayBuffer (never called from C++), ReadObject, ReadID.

Comment 2

[Andrew McCreight [:mccr8]]

There's some slightly weird code in nsXULPrototypeCache::BeginCaching() which does not immediately check for failure, but doesn't use the result of a ReadCString() until it checks for success, so it is fine. I considered refactoring the code, but it seems like it would be a lot of work for not much gain, if it could really be improved.

Comment 3

[Andrew McCreight [:mccr8]]

I split this into separate bugs, because they are in different parts of the code, and I'm not entirely sure what to do with all of these. But these filed bugs represent all of the problems I found in my manual code audit.