If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Improve ByteLengthIsValid()

RESOLVED FIXED in Firefox 47

Status

()

Core
IPC
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox47 fixed, firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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

a year ago
Created attachment 8746187 [details] [diff] [review]
part 1 - Reimplement ByteLengthIsValid using CheckedInt.

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

a year ago
Created attachment 8746188 [details] [diff] [review]
part 2 - Make ByteLengthIsValid failures fatal in release builds.

This will stop us from sending truncated messages, which will make the
receiver crash.
Attachment #8746188 - Flags: review?(nfroyd)
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+
Attachment #8746188 - Flags: review?(nfroyd) → review+

Comment 4

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f889750b4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b927c11679
Whiteboard: btpp-active

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c7f889750b4b
https://hg.mozilla.org/mozilla-central/rev/d0b927c11679
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 6

a year 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

a year ago
Attachment #8746188 - Flags: approval-mozilla-beta?
Attachment #8746188 - Flags: approval-mozilla-aurora?

Updated

a year ago
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+

Updated

a year ago
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

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f6c53848e73
https://hg.mozilla.org/releases/mozilla-aurora/rev/9fa876867aa8
status-firefox48: affected → fixed

Comment 9

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/35ff782279c5
https://hg.mozilla.org/releases/mozilla-beta/rev/4c0b0d24ab3b
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.