Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

integer overflow in /dom/html/reflection-embedded.html #11175

Open
metajack opened this issue May 13, 2016 · 3 comments
Open

integer overflow in /dom/html/reflection-embedded.html #11175

metajack opened this issue May 13, 2016 · 3 comments

Comments

@metajack
Copy link
Contributor

@metajack metajack commented May 13, 2016

This was originally found in #10544. The stack trace is:

Tests with unexpected results:
  ▶ CRASH [expected OK] /html/dom/reflection-embedded.html
  │ 
  │ thread 'ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }' panicked at 'arithmetic operation overflowed', /Users/servo/.cargo/registry/src/github.com-88ac128001ac3a9a/app_units-0.2.3/src/app_unit.rs:117
  │ stack backtrace:
  │    1:        0x1095fe0d8 - std::sys::backtrace::tracing::imp::write::h714760a4c8c0cdd8
  │    2:        0x109601d65 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hff309ab1d83ffd90
  │    3:        0x10960197c - std::panicking::default_hook::h08ad3bb09872855b
  │    4:        0x1095ec068 - std::sys_common::unwind::begin_unwind_inner::h406d5f1a330b854b
  │    5:        0x1095ed31e - std::sys_common::unwind::begin_unwind_fmt::h57ea3fbee1a40196
  │    6:        0x1095fd3a7 - rust_begin_unwind
  │    7:        0x109629700 - core::panicking::panic_fmt::ha6b3c19493c123b3
  │    8:        0x109629a5c - core::panicking::panic::h188c6c6a0fe5463c
  │    9:        0x10863db69 - script::dom::htmlimageelement::image_dimension_setter::h2e3eec119f489294
  │   10:        0x108206dd0 - script::dom::bindings::codegen::Bindings::HTMLImageElementBinding::set_width::h7f3877b6e131f649
  │   11:        0x108d9940a - CallJitSetterOp
  │   12:        0x108455ead - script::dom::bindings::utils::call_setter::hf3affdd67a62659e
  │   13:        0x108455d87 - script::dom::bindings::utils::generic_call::hfefc1ba5d4f4d4f0
  │   14:        0x108ed317b - _ZN2js6InvokeEP9JSContextN2JS8CallArgsENS_14MaybeConstructE
  │   15:        0x108ecb77d - _ZN2js6InvokeEP9JSContextRKN2JS5ValueES5_jPS4_NS2_13MutableHandleIS3_EE
  │   16:        0x108ee93ae - _ZN2js20InvokeGetterOrSetterEP9JSContextP8JSObjectN2JS5ValueEjPS5_NS4_13MutableHandleIS5_EE
  │   17:        0x108f06f77 - _ZN2js17NativeSetPropertyEP9JSContextN2JS6HandleIPNS_12NativeObjectEEENS3_IP8JSObjectEENS3_I4jsidEENS_13QualifiedBoolENS2_13MutableHandleINS2_5ValueEEERNS2_14ObjectOpResultE
  │   18:        0x108eeda70 - _ZN2js16SetObjectElementEP9JSContextN2JS6HandleIP8JSObjectEENS3_INS2_5ValueEEES8_bNS3_IP8JSScriptEEPh
  │   19:        0x108ffcc62 - _ZN2js3jitL17DoSetElemFallbackEP9JSContextPNS0_13BaselineFrameEPNS0_18ICSetElem_FallbackEPN2JS5ValueENS7_6HandleIS8_EESB_SB_
  └ fatal runtime error: Could not unwind stack, error = 5

This appears to be caused by clamping to UNSIGNED_LONG_MAX but then multiplying by 60 in Au::from_px.

@KiChjang
Copy link
Member

@KiChjang KiChjang commented May 13, 2016

I'm quite positive that I've seen a PR that resolves something similar to this by @pcwalton or @mbrubeck, but that was a while ago.

@metajack
Copy link
Contributor Author

@metajack metajack commented May 13, 2016

The spec seems to allow a range up to 2^32-1. This means our conversion to Au will overflow anything that needs 26 or more bits.

One fix is to clamp things like image width/height to (2^32-1) / 60 or to make Au an i64 instead of an i32.

I'm not sure what the correct behavior is here. Gecko's ns_coord is i32, but they seem to not track units explicitly, and I didn't find any clear examples of * AppUnitsPerCSSPixel() or * AppUnitsPerDevPixel(). It appears they are stored in CSS units, not AppUnits.

cc @pcwalton @dbaron

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented May 15, 2016

Maybe we should store this in CSS units too; can we ever get fractional data here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.