Closed Bug 1036782 Opened 5 years ago Closed 5 years ago

Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in js/src

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox32 --- wontfix
firefox33 --- verified

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

(Keywords: sec-want, Whiteboard: [adv-main33-])

Attachments

(2 files)

Replace calls to the deprecated MOZ_ASSUME_UNREACHABLE macro with MOZ_CRASH in js/src code that is not related to GC, JIT, or xpconnect. That code will be fixed by bug 1036780, bug 1036781, and bug 1036778, respectively.

Calls to MOZ_NOT_REACHED were replaced with MOZ_ASSUME_UNREACHABLE which, instead of crashing unconditionally, invokes undefined behavior in the name of dubious and dangerous compiler optimizations. See bug 990764 for details. This patch reverts to the original crashing behavior.

(MOZ_ASSUME_UNREACHABLE will eventually be removed or renamed in bug 990764.)
Attachment #8453547 - Flags: review?(jorendorff)
Comment on attachment 8453547 [details] [diff] [review]
replace-js-MOZ_ASSUME_UNREACHABLE.patch

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

Thank you so much for doing this.

::: js/src/frontend/Parser.cpp
@@ +5854,5 @@
>                                      pn);
>          }
>          return pn;
>      }
> +    MOZ_CRASH("unaryExpr");

Please just delete this one. The immediately preceding switch statement has a default: case, and all its cases end with a return statement.

On Clang and GCC, we build with -Werror=return-type, which means if we did have a mistake in one of the cases, the compiler would detect it and TBPL would go red. MOZ_CRASH here actually makes things worse, degrading the compile-time error to a runtime assertion.

::: js/src/irregexp/RegExpInterpreter.cpp
@@ +244,1 @@
>              break;

remove `break;` after MOZ_CRASH

::: js/src/jsfriendapi.cpp
@@ +998,5 @@
>          BaseShape::writeBarrierPre(static_cast<BaseShape*>(cell));
>      else if (kind == JSTRACE_TYPE_OBJECT)
>          types::TypeObject::writeBarrierPre((types::TypeObject *) ptr);
>      else
> +        MOZ_CRASH("invalid trace kind");

This is a GC thing; back it out unless you care to get add'l review from terrence.

::: js/src/jsfun.cpp
@@ +2019,5 @@
>          if (!obj)
>              return true;
>      }
>  
> +    MOZ_CRASH("Should not get here");

Delete it. Statically apparent.

::: js/src/jsinfer.cpp
@@ +182,5 @@
>              return "symbol";
>            case JSVAL_TYPE_MAGIC:
>              return "lazyargs";
>            default:
> +            MOZ_CRASH("Bad type");

jsinfer.{h,cpp} and jsinferinlines.h are really JIT stuff. Revert the changes or get add'l review.

::: js/src/jsinfer.h
@@ +169,5 @@
>          return "DefinitePropertiesAnalysis";
>        case ArgumentsUsageAnalysis:
>          return "ArgumentsUsageAnalysis";
>        default:
> +        MOZ_CRASH("Invalid ExecutionMode");

But it's OK to keep this one since it's debug-only.

::: js/src/jsinferinlines.h
@@ +43,5 @@
>      jit::IonScript *ion = jit::GetIonScript(script(), mode());
>      JS_ASSERT(ion != ION_COMPILING_SCRIPT);
>      return ion;
> +#else
> +    MOZ_CRASH("Invalid kind of CompilerOutput");

And it's OK to keep this one since it only happens when IonMonkey is disabled at configure time.

::: js/src/prmjtime.cpp
@@ +265,5 @@
>          // well.
>          needsCalibration = true;
>      }
>  
> +    MOZ_CRASH("Shouldn't get here");

Delete it. Statically apparent.

::: js/src/vm/StructuredClone.cpp
@@ +1282,5 @@
>        case Scalar::Uint8Clamped:
>          obj = JS_NewUint8ClampedArrayWithBuffer(context(), buffer, byteOffset, nelems);
>          break;
>        default:
> +        MOZ_CRASH("unknown TypedArrayObject type");

Ugh. The right thing to do here would be to report an error and return false!

Would you mind fixing this? Elsewhere in the file there are lots of places that use JS_ReportErrorNumber with JSMSG_SC_BAD_SERIALIZED_DATA, so all you have to do is copy one of those and make the string argument "unknown TypedArrayObject type".

@@ +1329,1 @@
>      }

If you want, you can just delete this whole function and change the only caller

    uint32_t nbytes = nelems * bytesPerTypedArrayElement(arrayType);

to say

    uint32_t nbytes = nelems << TypedArrayShift(arrayType);

This is what the other places that need to compute this value are already doing.
Attachment #8453547 - Flags: review?(jorendorff) → review+
Landed without JIT, GC, or StructuredClone.cpp changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f7b0e89bcf
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> ::: js/src/vm/StructuredClone.cpp
> @@ +1282,5 @@
> >        case Scalar::Uint8Clamped:
> >          obj = JS_NewUint8ClampedArrayWithBuffer(context(), buffer, byteOffset, nelems);
> >          break;
> >        default:
> > +        MOZ_CRASH("unknown TypedArrayObject type");
> 
> Ugh. The right thing to do here would be to report an error and return false!
> 
> Would you mind fixing this? Elsewhere in the file there are lots of places
> that use JS_ReportErrorNumber with JSMSG_SC_BAD_SERIALIZED_DATA, so all you
> have to do is copy one of those and make the string argument "unknown
> TypedArrayObject type".

I think StructuredClone.cpp's "unknown TypedArrayObject type" code paths really are unreachable. The readTypedArray function range checks arrayType and reports JSMSG_SC_BAD_SERIALIZED_DATA before calling the switch statement or readV1ArrayBuffer:

https://hg.mozilla.org/mozilla-central/file/tip/js/src/vm/StructuredClone.cpp#l1227

Do you still want me to change the readTypedArray's and readV1ArrayBuffer's default cases from MOZ_CRASH to JS_ReportErrorNumber?
Flags: needinfo?(jorendorff)
https://hg.mozilla.org/mozilla-central/rev/f8f7b0e89bcf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(In reply to Chris Peterson (:cpeterson) from comment #3)
> (In reply to Jason Orendorff [:jorendorff] from comment #1)
> > ::: js/src/vm/StructuredClone.cpp
> > @@ +1282,5 @@
> > >        case Scalar::Uint8Clamped:
> > >          obj = JS_NewUint8ClampedArrayWithBuffer(context(), buffer, byteOffset, nelems);
> > >          break;
> > >        default:
> > > +        MOZ_CRASH("unknown TypedArrayObject type");
> > 
> > Ugh. The right thing to do here would be to report an error and return false!
> > 
> > Would you mind fixing this? Elsewhere in the file there are lots of places
> > that use JS_ReportErrorNumber with JSMSG_SC_BAD_SERIALIZED_DATA, so all you
> > have to do is copy one of those and make the string argument "unknown
> > TypedArrayObject type".
> 
> I think StructuredClone.cpp's "unknown TypedArrayObject type" code paths
> really are unreachable. The readTypedArray function range checks arrayType
> and reports JSMSG_SC_BAD_SERIALIZED_DATA before calling the switch statement
> or readV1ArrayBuffer:
> 
> https://hg.mozilla.org/mozilla-central/file/tip/js/src/vm/StructuredClone.
> cpp#l1227
> 
> Do you still want me to change the readTypedArray's and readV1ArrayBuffer's
> default cases from MOZ_CRASH to JS_ReportErrorNumber?

No. Thanks for the explanation ... I'd be thrilled if you'd add a comment there or just change the string from "unknown TypedArrayObject type" to something more like "can't get here - already checked for bad types in readTypedArray".
Flags: needinfo?(jorendorff)
Keywords: sec-want
Added MOZ_CRASH with comments, removed bytesPerTypedArrayElement, and fixed some indentation.
Attachment #8456609 - Flags: review?(jorendorff)
Comment on attachment 8456609 [details] [diff] [review]
part-2-StructuredClone.patch

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

Thanks again.

::: js/src/vm/StructuredClone.cpp
@@ +1221,5 @@
>  }
>  
>  bool
> +JSStructuredCloneReader::readTypedArray(uint32_t arrayType, uint32_t nelems,
> +                                        Value *vp, bool v1Read)

Lines up to 99 columns are OK in SpiderMonkey.

@@ +1317,5 @@
>  {
>      JS_ASSERT(arrayType <= Scalar::Uint8Clamped);
>  
> +    uint32_t nbytes =
> +        nelems << TypedArrayShift(static_cast<Scalar::Type>(arrayType));

This fits on one line too.
Attachment #8456609 - Flags: review?(jorendorff) → review+
Whiteboard: [adv-main33-]
Verified change in Fx33, 2014-10-06.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.