Message routing ID overflow?

RESOLVED FIXED in Firefox 68

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: jld, Assigned: Srujana.htt121)

Tracking

({good-first-bug})

Trunk
mozilla68
Points:
---

Firefox Tracking Flags

(firefox44 wontfix, firefox68 fixed)

Details

Attachments

(1 attachment)

We seem to be allocating IPC message routing IDs (a.k.a. actor IDs) by incrementing an int32_t with no provision for handling overflow.  Currently it's hard to do billions of anything that creates an actor fast enough to demonstrate problems — I tried using failed image loads, but IndexedDB transactions might work ‘better’ — so this might not matter now, but it's the kind of thing that probably ought to have a bug on file.
Shmem::id_t is also 32-bit and per-toplevel-protocol and managed similarly to actor IDs.
Priority: -- → P3
potential helper fix - add an release assert when we hit this.
Keywords: good-first-bug
For future reference, this is the increment/decrement that might overflow: https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/ipc/glue/ProtocolUtils.cpp#809

And here's the similar usage for Shmem: https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/ipc/glue/ProtocolUtils.cpp#872

It should be simple to change those counters to use our CheckedInt class, and then release assert that the result of the increment/decrement is valid before extracting the value with CheckedInt::value.


Once we ship that, if we see crash reports for hitting that assertion, then we can consider either widening to 64 bits or reusing old values.  (Changing the routing ID type would change the message header layout and we'd need to think about how that affects detecting build ID mismatches; see also bug 1366808.)
Hi Jed,

I'd like to take this up and work on it. So, is the process as simple as changing the type from int32_t to CheckedInt? Also, could I get a few more details about what exactly we're trying to assert?


Thanks!

Hi, I am working with David Parks. he assigned me this bug. Can anyone add me?

This is what I have understood. Using Checkeedint, we need to update the variables(ie incrementing mLastLocalId).

We need to assert if it is overflowing or not so that depending on the crash reports, we can later convert 32 bit int to 64 bit int.

I have submitted a patch assuming the above. Please review it and let me know any changes.

Assignee: nobody → Srujana.htt121
Attachment #9051689 - Attachment description: Bug 1212103 : Message routing ID overflow - Added CheckedInt assert. → Bug 1212103 : Added CheckedInt assert.
Attachment #9051689 - Attachment description: Bug 1212103 : Added CheckedInt assert. → Bug 1212103 : Added assert to check if the id is exceeding bounds
Attachment #9051689 - Attachment description: Bug 1212103 : Added assert to check if the id is exceeding bounds → Bug 1212103 : Added assert to check if the id is exceeding bounds of signed 32 bit integer

Can you please review my bug? I am apply for outreachy and the deadline is 26th March. Thank you

Flags: needinfo?(jld)
Keywords: checkin-needed

I have queued the landing of this patch. At some point, it should automatically land, then later somebody will merge the patch to mozilla-central and the bug will be closed.

Keywords: checkin-needed
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d97860471b4
Added assert to check if the id is exceeding bounds of signed 32 bit integer r=jld
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Flags: needinfo?(jld)
You need to log in before you can comment on or make changes to this bug.