Closed Bug 1268130 Opened 4 years ago Closed 4 years ago

Improve ByteLengthIsValid()

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: btpp-active)

Attachments

(2 files)

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.
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)
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+
Whiteboard: btpp-active
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?
Attachment #8746188 - Flags: approval-mozilla-beta?
Attachment #8746188 - Flags: approval-mozilla-aurora?
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+
You need to log in before you can comment on or make changes to this bug.