Closed
Bug 1268130
Opened 8 years ago
Closed 8 years ago
Improve ByteLengthIsValid()
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Whiteboard: btpp-active)
Attachments
(2 files)
5.14 KB,
patch
|
froydnj
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
froydnj
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This method has some problems. They may be causing bug 1265680. 1. We should crash if this check fails when doing a Write(), or we'll just send a truncated message, which will cause the receiver to crash. 2. There's a check that says "nsTArray only handles sizes up to INT32_MAX", but AFAICT this isn't true. Instead, the restriction as far as I can see it is that EnsureCapacity() succeeds, which in turn requires that the _capacity_ not exceed the _unsigned int_ limit. This is sort of checked in the next part, but again it is using INT32_MAX rather than UINT32_MAX. I feel like these checks should be limited to making sure the Write/ReadBytes operation isn't going haywire, and enforcing internal nsTArray invariants should be done be left to those being done by the data structure itself, using some fallible variant. 3. This is using some kind of hand-rolled overflow checking, but it should really use CheckedInt.
Assignee | ||
Comment 1•8 years ago
|
||
I outlined ByteLengthIsValid, and made it outside the template, to reduce code size and compilation time, but I can undo that if it doesn't seem worthwhile. The first check in ByteLengthIsValid() says "nsTArray only handles sizes up to INT32_MAX", but the actual requirement is that the capacity is no larger than UINT32_MAX. The check is overly restrictive if sizeof(E) is 1 byte, and overly permissive if sizeof(E) is greater than 2 bytes. I removed this check. Internal nsTArray invariants should be enforced by nsTArray methods. The second check is trying to check for overflow, but that should just be done using CheckedInt. I'm still waiting on the try run, but this doesn't seem too risky to me. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65e6e4946bb0
Attachment #8746187 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•8 years ago
|
||
This will stop us from sending truncated messages, which will make the receiver crash.
Attachment #8746188 -
Flags: review?(nfroyd)
Comment 3•8 years ago
|
||
Comment on attachment 8746187 [details] [diff] [review] part 1 - Reimplement ByteLengthIsValid using CheckedInt. Review of attachment 8746187 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/glue/IPCMessageUtils.cpp @@ +6,5 @@ > + > +#include "IPCMessageUtils.h" > +#include "mozilla/CheckedInt.h" > + > +bool IPC::ByteLengthIsValid(uint32_t aNumElements, size_t aElementSize, int* aByteLength) Nit: I think the typical way we do this is: namespace IPC { bool ByteLengthIsValid(...) } // namespace IPC ::: ipc/glue/IPCMessageUtils.h @@ -442,5 @@ > - // dance. > - static bool ByteLengthIsValid(size_t aNumElements, int* aTotalLength) { > - static_assert(sizeof(int) == sizeof(int32_t), "int is an unexpected size!"); > - > - // nsTArray only handles sizes up to INT32_MAX. I don't know why I thought this should be done manually and not using CheckedInt.
Attachment #8746187 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8746188 -
Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f889750b4b https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b927c11679
Updated•8 years ago
|
Whiteboard: btpp-active
Comment 5•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c7f889750b4b https://hg.mozilla.org/mozilla-central/rev/d0b927c11679
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8746187 [details] [diff] [review] part 1 - Reimplement ByteLengthIsValid using CheckedInt. Approval Request Comment [Feature/regressing bug #]: e10s [User impact if declined]: These two patches improve error handling and reporting in IPC code, which might help some known e10s crashes. I have no evidence that this eliminated any crashes, though. [Describe test coverage new/current, TreeHerder]: this code runs frequently [Risks and why]: fairly low. It replaces some questionable code, and changes one crash into another. [String/UUID change made/needed]: none
Attachment #8746187 -
Flags: approval-mozilla-beta?
Attachment #8746187 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8746188 -
Flags: approval-mozilla-beta?
Attachment #8746188 -
Flags: approval-mozilla-aurora?
status-firefox47:
--- → affected
status-firefox48:
--- → affected
Comment on attachment 8746187 [details] [diff] [review] part 1 - Reimplement ByteLengthIsValid using CheckedInt. e10s stability related, baked in Nightly for a bit, Aurora48+, Beta47+
Attachment #8746187 -
Flags: approval-mozilla-beta?
Attachment #8746187 -
Flags: approval-mozilla-beta+
Attachment #8746187 -
Flags: approval-mozilla-aurora?
Attachment #8746187 -
Flags: approval-mozilla-aurora+
Attachment #8746188 -
Flags: approval-mozilla-beta?
Attachment #8746188 -
Flags: approval-mozilla-beta+
Attachment #8746188 -
Flags: approval-mozilla-aurora?
Attachment #8746188 -
Flags: approval-mozilla-aurora+
Comment 8•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f6c53848e73 https://hg.mozilla.org/releases/mozilla-aurora/rev/9fa876867aa8
Comment 9•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/35ff782279c5 https://hg.mozilla.org/releases/mozilla-beta/rev/4c0b0d24ab3b
You need to log in
before you can comment on or make changes to this bug.
Description
•