969309 - Assertion failure: masm.currentOffset() - lastOsiPointOffset_ >= Assembler::patchWrite_NearCallSize(), at jit/shared/CodeGenerator-shared.cpp:423

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Testcase for shell

The attached testcase asserts on mozilla-central revision f550b112a19b (x86 ARM simulator build, run with --fuzzing-safe --ion-eager).

Comment 1

[Jan de Mooij [:jandem]]

Douglas or Marty, can one of you take this?

Comment 2

[Douglas Crosher [:dougc]]

fwiw this does not reproduce on my local development build which has some pending fixes.

This assertion is not compiled when the assembler buffer patch, bug 760642, is applied so might be addressed there.  I am look over this code so will keep it in mind.

Comment 3

[Nicolas B. Pierron [:nbp]]

This assertion is here to prevent random code execution to prevent patching the call instructions with the invalidation code.

patchWrite_NearCallSize() returns the size needed for making 2 consecutive calls in the generated code, such as the backward patching on a call does not affect the previous instruction execution, and leave enough space for the OSR jump.

If this assert raised, this means that the previous instruction is closer than expected. (less than 4 bytes on ARM).

Comment 4

[Marty Rosenberg [:mjrosenb]]

right.  "less than 4 bytes", that is basically impossible.  Also the fact that this only reproduces a handful of times in 100 runs (I have yet to actually reproduce it) makes me believe that the assertion is just indicating that something else has gone wrong.

Comment 5

[Marty Rosenberg [:mjrosenb]]

This seems pretty bogus.  When we run out of memory, we stop adding instructions to the assembly buffer.  So, when people query the assembly buffer for how much stuff is in it, it keeps giving the same answer, which then makes this check a bit sad.  For the most part we're doing the right thing with this OOM, but the post-oom world isn't as sane as assertions want it to be.  I can paper over this by just not asserting if the buffer believes it has run out of memory, or I can try something else.  We can probably also safely ignore this.

Comment 6

[Daniel Veditz [:dveditz]]

> We can probably also safely ignore this.

You're saying this is not a security bug and we should WONTFIX it?

Comment 7

[Andrew McCreight [:mccr8]]

ni Marty for comment 6.

Comment 8

[Marty Rosenberg [:mjrosenb]]

Yeah, that is my opinion.  If someone thinks this warrants papering over, I can do that, but it looks incredibly tame and unlucky.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #8)
> Yeah, that is my opinion.  If someone thinks this warrants papering over, I
> can do that, but it looks incredibly tame and unlucky.

I think it'd be good to do that (only assert if non-OOM), to make it easier for the fuzzers to find real, non-OOM problems.

Comment 10

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Error: Failed to compile specified revision f550b112a19b (maybe try another?)

Comment 11

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, failed due to error: Error: Failed to compile specified revision f550b112a19b (maybe try another?)

Comment 12

[Sean Stangl [:sstangl]]

Attachment: 0001-Bug-969309-Avoid-write-patch-check-on-OOM.-r.patch

Testcase hasn't reproduced for a year, but might as well, since the error still theoretically exists.

Comment 13

[Hannes Verschore [:h4writer]]

Comment on attachment 8627917 [details] [diff] [review]
0001-Bug-969309-Avoid-write-patch-check-on-OOM.-r.patch

Review of attachment 8627917 [details] [diff] [review]:
-----------------------------------------------------------------

Indeed good to not assert for known invalid state!

Comment 14

[Sean Stangl [:sstangl]]

Apparently I landed this last week, but the auto-comment bot doesn't function on s-s bugs.

https://hg.mozilla.org/mozilla-central/rev/63e4d75cc007