(In reply to Iain Ireland [:iain] from comment #2) > I think the fix is probably to somehow beef up ensureOsiSpace? Yes. This is the way! `ensureOsiSpace` is used to ensure that the code that we generate for invalidation (i-e. a bailout) does not overlap with the return address of the call of another instruction. Otherwise, we might have random code execution. [overlapping-patches] When invalidated, we patch the code and store the delta-offset from the return location to the invalidation epilogue. This is useful as if we lose the reference of the current function, then the space might be reclaimed and reused before we return to the invalidated function, and we might once again have random code execution. [exec-after-free] Ion is limited, It can execute only a single potentially-invalidating call per LIR instruction. If we have more than one, then we risk returning to an invalidated code base where non-code (the delta-offset to the invalidation offset) is written in place of the code. Which can be assimilated to random code execution (except that it is limited to small offsets). [exec-after-patch] However, we have not forbidden generating multiple potentially-invalidating calls in the same LIR, as we already had use cases for multiple calls on different branches, which is safe as long as we do not re-enter a potentially-invaliding call before reaching the end of the LIR instruction. (In reply to Iain Ireland [:iain] from comment #2) > This is obviously not supposed to happen, but I can't find the code that prevents it. We have [ensureOsiSpace](https://searchfox.org/mozilla-central/rev/4fcb729dc98138af00570c0a378fbff90a167d79/js/src/jit/shared/CodeGenerator-shared.cpp#809) that seems like it's intended to avoid this, but AFAICT that only prevents OSI patching from being clobbered, not the delta. (It also looks like we never actually generate any nops using that code.) On RISCV this might be another story. I suspect the problem you are facing is that `ensureOsiSpace` only inserts nop-slides if the last `ensureOsiSpace` was not too close, but at the moment it is only called under `LOsiPoint`, which is the problem. All potentially invalidating call should be checking that `lastOsiPointOffset_` is far away enough (by calling `ensureOsiSpace`) to insert a nop-slide to safely encode the bailing jump (`PatchWrite_NearCall`). Which is most likely the problem you are seeing if a potentially-invalidating call instruction is generated directly after an `LOsiPoint`: LOsiPoint LPotentiallyInvalidatingCall Calling `ensureOsiPoint()` before the call instruction of `LPotentiallyInvalidatingCall` should do the trick ;)
Bug 1814899 Comment 3 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(In reply to Iain Ireland [:iain] from comment #2) > I think the fix is probably to somehow beef up ensureOsiSpace? Yes. This is the way! `ensureOsiSpace` is used to ensure that the code that we generate for invalidation (i-e. a bailout) does not overlap with the return address of the call of another instruction. Otherwise, we might have random code execution. [overlapping-patches] When invalidated, we patch the code and store the delta-offset from the return location to the invalidation epilogue. This is useful as if we lose the reference of the current function, then the space might be reclaimed and reused before we return to the invalidated function, and we might once again have random code execution. [exec-after-free] Ion is limited, It can execute only a single potentially-invalidating call per LIR instruction. If we have more than one, then we risk returning to an invalidated code base where non-code (the delta-offset to the invalidation offset) is written in place of the code. Which can be assimilated to random code execution (except that it is limited to small offsets). [exec-after-patch] However, we have not forbidden generating multiple potentially-invalidating calls in the same LIR, as we already had use cases for multiple calls on different branches, which is safe as long as we do not re-enter a potentially-invaliding call before reaching the end of the LIR instruction. (In reply to Iain Ireland [:iain] from comment #2) > This is obviously not supposed to happen, but I can't find the code that prevents it. We have [ensureOsiSpace](https://searchfox.org/mozilla-central/rev/4fcb729dc98138af00570c0a378fbff90a167d79/js/src/jit/shared/CodeGenerator-shared.cpp#809) that seems like it's intended to avoid this, but AFAICT that only prevents OSI patching from being clobbered, not the delta. (It also looks like we never actually generate any nops using that code.) On RISCV this might be another story. I suspect the problem you are facing is that `ensureOsiSpace` only inserts nop-slides if the last `ensureOsiSpace` was not too close, but at the moment it is only called under `LOsiPoint`, which is the problem. All potentially invalidating call should be checking that `lastOsiPointOffset_` is far away enough (by calling `ensureOsiSpace`) to insert a nop-slide to safely encode the bailing jump (`PatchWrite_NearCall`). Which is most likely the problem you are seeing if a potentially-invalidating call instruction is generated directly after an `LOsiPoint`: ``` LOsiPoint LPotentiallyInvalidatingCall ``` Calling `ensureOsiPoint()` before the call instruction of `LPotentiallyInvalidatingCall` should do the trick ;)
(In reply to Iain Ireland [:iain] from comment #2) > I think the fix is probably to somehow beef up ensureOsiSpace? Yes. This is the way! `ensureOsiSpace` is used to ensure that the code that we generate for invalidation (i-e. a bailout) does not overlap with the return address of the call of another instruction. Otherwise, we might have random code execution. [overlapping-patches] When invalidated, we patch the code and store the delta-offset from the return location to the invalidation epilogue. This is useful as if we lose the reference of the current function, then the space might be reclaimed and reused before we return to the invalidated function, and we might once again have random code execution. [exec-after-free] Ion is limited, It can execute only a single potentially-invalidating call per LIR instruction. If we have more than one, then we risk returning to an invalidated code base where non-code (the delta-offset to the invalidation offset) is written in place of the code. Which can be assimilated to random code execution (except that it is limited to small offsets). [exec-after-patch] However, we have not forbidden generating multiple potentially-invalidating calls in the same LIR, as we already had use cases for multiple calls on different branches, which is safe as long as we do not re-enter a potentially-invaliding call before reaching the end of the LIR instruction. (In reply to Iain Ireland [:iain] from comment #2) > This is obviously not supposed to happen, but I can't find the code that prevents it. We have [ensureOsiSpace](https://searchfox.org/mozilla-central/rev/4fcb729dc98138af00570c0a378fbff90a167d79/js/src/jit/shared/CodeGenerator-shared.cpp#809) that seems like it's intended to avoid this, but AFAICT that only prevents OSI patching from being clobbered, not the delta. (It also looks like we never actually generate any nops using that code.) On RISCV this might be another story. I suspect the problem you are facing is that `ensureOsiSpace` only inserts nop-slides if the last `ensureOsiSpace` was not too close, but at the moment it is only called under `LOsiPoint`, which is the problem. All potentially invalidating call should be checking that `lastOsiPointOffset_` is far away enough (by calling `ensureOsiSpace`) to insert a nop-slide to safely encode the bailing jump (`PatchWrite_NearCall`). Which is most likely the problem you are seeing if a potentially-invalidating call instruction is generated directly after an `LOsiPoint`: ``` LOsiPoint LPotentiallyInvalidatingCall ``` Calling `ensureOsiSpace()` before the call instruction of `LPotentiallyInvalidatingCall` should do the trick ;)