1212103 - Message routing ID overflow?

Status: RESOLVED FIXED

(Core :: IPC, defect, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Shmem::id_t is also 32-bit and per-toplevel-protocol and managed similarly to actor IDs.

Comment 2

[Jim Mathies [:jimm]]

potential helper fix - add an release assert when we hit this.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.)

Comment 4

[Vedant]

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!

Comment 5

[Srujana Peddinti [:srujana121]]

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.

Comment 6

[Srujana Peddinti [:srujana121]]

Attachment: Bug 1212103 : Added assert to check if the id is exceeding bounds of signed 32 bit integer

Comment 7

[Srujana Peddinti [:srujana121]]

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

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9d97860471b4