eliminate usage of base serialization functions for values without strict sizes

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

9 years ago
We should eliminate usage of the following functions from our ipc code because they deal with values that don't have the same size on all architectures.

[Read/Write]Long
[Read/Write]ULong
[Read/Write]Size
[Read/Write]IntPtr

We should also move "int" and "short" versions of these functions to more explicitly-sized versions but they happen to be the same size on i386 and x86_64 so they are less of a priority.
(Assignee)

Updated

9 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

9 years ago
Blocks: 559142

Comment 1

9 years ago
The easiest way to do this is presumably to remove the functions entirely, right?
We could temporarily hijack these functions to read/write the i386 size types regardless of the architecture with a warning/abort on truncation. This would let us progress around this problem while the references are being removed and would also find the places where the i386 types are too small.

That said I agree that the desired long term fix is to remove these functions entirely.
(Assignee)

Comment 3

9 years ago
I had this patch around but I hadn't tested it yet when I filed this bug. It is somewhat more difficult to just kill the functions because they end up being used implicitly by typedef'd param types so I coded it up such that variable sized types are always serialized as the larger type. This way a 64-bit -> 64-bit message could use the whole range of the type, but the downside is that if a value requiring the full 64 type is sent from a 64-bit arch to a 32-bit arch it'll get incorrectly truncated. I don't think this will be a problem in practice.
(Assignee)

Comment 4

9 years ago
Posted patch fix v1.1 (obsolete) — Splinter Review
Attachment #468763 - Attachment is obsolete: true
Attachment #468773 - Flags: review?(jones.chris.g)
> if a value requiring the full 64 type is sent from a 64-bit arch to a 32-bit
> arch it'll get incorrectly truncated. I don't think this will be a problem in
> practice.

It should be easy to detect truncation and throw a warning (or abort). This would make errors more obvious and make it easy to trace what messages require the larger 64-bit types.
(Assignee)

Comment 6

9 years ago
Yeah, we should add debug assertions. I'll do that in the next patch revision.
Comment on attachment 468773 [details] [diff] [review]
fix v1.1

r=me with overflow guard asserts.  Now we get to see which platforms give us |long| handles > 1<<31 :/.
Attachment #468773 - Flags: review?(jones.chris.g) → review+
(Assignee)

Updated

9 years ago
blocking2.0: --- → beta6+
(Assignee)

Comment 8

9 years ago
Posted patch fix v1.2 (obsolete) — Splinter Review
Attachment #468773 - Attachment is obsolete: true
(Assignee)

Comment 9

9 years ago
Posted patch fix v1.3 (obsolete) — Splinter Review
Attachment #469582 - Attachment is obsolete: true
(Assignee)

Comment 10

9 years ago
Posted patch fix v1.4Splinter Review
Attachment #469645 - Attachment is obsolete: true
(Assignee)

Comment 11

9 years ago
Comment on attachment 469691 [details] [diff] [review]
fix v1.4

There are enough changes here that I'd like to get this reviewed again.
Attachment #469691 - Flags: review?(jones.chris.g)
Comment on attachment 469691 [details] [diff] [review]
fix v1.4

Nice.
Attachment #469691 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 13

9 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/da862772518d
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.