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

NEW
Unassigned

Status

()

Core
XUL
3 years ago
9 months ago

People

(Reporter: mccr8, Unassigned)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected)

Details

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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.
Summary: Don't use return values of failing calls in nsXULPrototypeElement::Deserialize() → Callers of nsIObjectInputStream methods should not use the result of failed Read*() methods
(Reporter)

Comment 2

3 years ago
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.
Assignee: nobody → continuation
(Reporter)

Updated

3 years ago
Depends on: 1193564
(Reporter)

Updated

3 years ago
Depends on: 1193567
(Reporter)

Updated

3 years ago
Depends on: 1193572
(Reporter)

Updated

3 years ago
Depends on: 1193578
(Reporter)

Comment 3

3 years ago
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.
(Reporter)

Updated

9 months ago
Assignee: continuation → nobody
You need to log in before you can comment on or make changes to this bug.