559592 - pointer mayhem in ARM platform detection

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Crowder]

The patch says it all.  1.9.2 is also affected by this issue.

Comment 1

[Brian Crowder]

Attachment: The fix

Comment 2

[Brian Crowder]

Comment on attachment 439270 [details] [diff] [review]
The fix

Moving review to vlad since it's vlad's code and jorendorff wrote the patch.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 439270 [details] [diff] [review]
The fix

Pretty sure this isn't correct -- the a_un union used to contain a_ptr, but that doesn't work due to 32-bit/64-bit issues, so casting a_val to a pointer should be the right thing.

Taking its address implies that the string is encoded directly in the structure, which would mean that sizeof(Elf32_auxv_t) isn't constant.

Comment 4

[Brian Crowder]

No, the sizeof(Elf32_auxv_t) remains constant as long as the strings aren't expected to be null-terminated and are 4-bytes-or-less.  In this case, according to the comment here:

http://mxr.mozilla.org/mozilla-central/source/js/src/jstracer.cpp#7337

they could even be null-terminated, since only 3 bytes seem to be necessary for the platform encoding.

Comment 5

[Brian Crowder]

Attachment: the REAL fix

Okay, we're already ignoring this string in scratchbox.  Let's do a BETTER job of ignoring it.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 439328 [details] [diff] [review]
the REAL fix

Needs to be broader than this.. none of auxv is valid when running under qemu.  So should be something like:

if (!scratchbox) {
  open & read auxv
} else {
  if (getenv("ARM_FORCE_HWCAP")) {
  ...
  } else {
     neon = false;
     ...;
  }
}

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

er actually, could simplify a bit more and check the ARM_FORCE_HWCAP and ARM_FORCE_PLATFORM env vars at the end, after we've initialized either from auxv or just hardcoded scratchbox values for armv4.

Comment 8

[Brian Crowder]

Attachment: ignore auxv values in scratchbox

I think this is the cleanest way to do this without duplicating code or refactoring this function more than it deserves.

Comment 9

[Brian Crowder]

Attachment: with better comments.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 439339 [details] [diff] [review]
with better comments.

Yep, looks fine -- thanks!

Comment 11

[Brian Crowder]

Comment on attachment 439339 [details] [diff] [review]
with better comments.

We need this on 1.9.2 also

Comment 12

[Brian Crowder]

http://hg.mozilla.org/mozilla-central/rev/9cdef34053c2

Comment 13

[LegNeato]

Comment on attachment 439339 [details] [diff] [review]
with better comments.

a=LegNeato for 1.9.2.5. Please *only* land it on mozilla-1.9.2 default, as we are still working on the 3.6.4 relbranch and do not want this landed there

Comment 14

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1fcff072ea16