Closed Bug 1212103 Opened 5 years ago Closed 1 year ago
Message routing ID overflow?
47 bytes, text/x-phabricator-request
|Details | Review|
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.
potential helper fix - add an release assert when we hit this.
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!
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
Pushed by email@example.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
You need to log in before you can comment on or make changes to this bug.