Closed Bug 1181142 Opened 9 years ago Closed 8 years ago

crash in strncpy | js::DecompileValueGenerator(JSContext*, int, JS::Handle<T>, JS::Handle<T>, int)

Categories

(Core :: JavaScript Engine, defect)

38 Branch
x86
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: philipp, Assigned: erahm)

References

Details

(Keywords: crash, topcrash-win)

Crash Data

Attachments

(5 files, 3 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-f04217f0-990d-4b5b-93c0-fc4092150707.
=============================================================

filing this bug because there is no associated report to that signature yet. some more information about the circumstances of this crash are available in a thread at https://support.mozilla.org/en-US/questions/1070827 where the user posted about this problem.
(Don't trust the stack for this one, bug 1181148)

The strcmp fell off the end of a page:
            if (strcmp(result, "(intermediate value)"))
|result| is 0x29d1fffe and its memory mapping stops at 0x29d20000. The CRT is trying to be clever and compare four bytes at a time, since the length of the string is 0x15.

Is it legit for |result| to have such a value? If so then I guess we'll need to work around this.
Flags: needinfo?(luke)
It looks like 'result' ends up receiving memory malloc'd by js_malloc (which I think is just the global malloc).  Is '*result' == 0?  If I'm reading you correctly, that would be the only valid input.  If so, I wonder if the builtin msvcrt malloc always allocates a minimum of 4 bytes and so we're breaking that assumption with jemalloc?
Flags: needinfo?(luke)
> Is '*result' == 0?  If I'm reading you
> correctly, that would be the only valid input.

Not sure I understand. As far as I can tell, either |result| is a 1-byte allocation consisting of just a null-terminator, or a 2-byte allocation consisting of any random char followed by a null.

> If so, I wonder if the
> builtin msvcrt malloc always allocates a minimum of 4 bytes and so we're
> breaking that assumption with jemalloc?

Yeah, could be.
Oops, I was thinking of 'jschar' (two bytes), but I see this is a normal 'char'.  Ok, so it could be a 0 or 1-length null-terminated string.
(In reply to Luke Wagner [:luke] from comment #2)
> It looks like 'result' ends up receiving memory malloc'd by js_malloc (which
> I think is just the global malloc).  Is '*result' == 0?  If I'm reading you
> correctly, that would be the only valid input.  If so, I wonder if the
> builtin msvcrt malloc always allocates a minimum of 4 bytes and so we're
> breaking that assumption with jemalloc?

On linux and OS X, bug 691003 modified the behavior to make the minimum alignment 4 bytes (8 on 64-bit). We decided not to do Windows, but I wonder if we should.

In this case we could do something more clever: return the size of result from |DecompileExpressionFromStack| and then directly use memcmp (not strcmp) iff size == 15.
(In reply to Eric Rahm [:erahm] from comment #5)
> On linux and OS X, bug 691003 modified the behavior to make the minimum
> alignment 4 bytes (8 on 64-bit). We decided not to do Windows, but I wonder
> if we should.

If we can confirm it is breaking widespread msvcrt assumptions, that might be the right fix.

> In this case we could do something more clever: return the size of result
> from |DecompileExpressionFromStack| and then directly use memcmp (not
> strcmp) iff size == 15.

Patches welcome, but this is pretty cold code :)
Bug 691003 comment 0:

> The Linux ABI specifies that memory returned by malloc and friends is word
> aligned at minimum. (4 bytes on 32-bit, 8 bytes on 64-bit.)
> 
> GCC knows this, and its value range propagation pass will carry this
> information around from malloc and company to things like strlen or strcpy.
> 
> When gcc decides to inline the string functions, if it knows that the memory
> they're operating on is word aligned, it will skip the usual byte-at-a-time
> preamble and start with word-at-a-time accesses.
> 
> This will cause a crash if you allocate e.g. two bytes and jemalloc decides
> to place them in the last two bytes of a page before an unmapped region.

Gee, sounds familiar :)
Bug 691003 made the minimum allocation size word sized for Linux and OS X, we
now need to do this on Windows as well.
Attachment #8630719 - Flags: review?(mh+mozilla)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8630719 [details] [diff] [review]
Make the minumum allocation size word sized on all platforms

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

::: memory/mozjemalloc/jemalloc.c
@@ +551,1 @@
>  #define TINY_MIN_2POW           (sizeof(void*) == 8 ? 3 : 2)

If we're worried about the compiler assuming things based on what its associated CRT does, then this isn't going to cut it:

from https://msdn.microsoft.com/en-us/library/6ewkz86d%28v=vs.120%29.aspx:
"(In Visual C++, the fundamental alignment is the alignment that's required for a double, or 8 bytes. In code that targets 64-bit platforms, it’s 16 bytes)"

The "fun" part is that this is likely to regress our memory usage quite severely (iirc from jemalloc3, which didn't have the same minimum size).

The same change should be done to jemalloc3 too.
Attachment #8630719 - Flags: review?(mh+mozilla) → review-
Bug 691003 made the minimum allocation size word sized for Linux and OS X, we
now need to do a similar change on Windows as well. For Windows the requirement
is double-aligned, so 8-bytes on 32-bit and 16-bytes on 64-bit.
Attachment #8630719 - Attachment is obsolete: true
I've updated the patch to use 8/16 bytes on Windows. I have two talos runs pending:
 - baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0c65e7f351a
 - w/ patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2114b86181

I will be out for the rest of the week, so if someone else wants to finish this off before I get back they are more than welcome to.
(as glandium said, we'll need to update jemalloc3 as well)
Comment on attachment 8630810 [details] [diff] [review]
Make the minumum allocation size word sized on all platforms

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

::: memory/mozjemalloc/jemalloc.c
@@ +545,5 @@
>  #define	CACHELINE		((size_t)(1U << CACHELINE_2POW))
>  
>  /*
> + * Smallest size class to support.  On Windows the smallest allocation size
> + * must be the size of a double (8 bytes on 32-bit, 16 bytes on 64-bit). On

I would hope that a double is still 8 bytes on 64-bit! I suspect the reason for 16-byte alignment is to accommodate some other type. Maybe just say "must be 8 bytes on 32-bit, 16 bytes on 64-bit"?
Do we really need to go to 8/16 on Windows? I know that's what the docs say but the problematic case is 1 or 2 bytes. In theory Linux requires 8/16 as well (see the start of http://www.gnu.org/software/libc/manual/html_node/Aligned-Memory-Blocks.html) but 4/8 is working fine.
What matters is not what glibc claims it does, but what POSIX guarantees, which is, according to
http://pubs.opengroup.org/onlinepubs/009695399/functions/malloc.html
that "The pointer returned if the allocation succeeds shall be suitably aligned so that it may be assigned to a pointer to any type of object and then used to access such an object in the space allocated". That means 4/8.

I doubt gcc/clang will optimize based on what glibc actually does, as opposed to what POSIX says, especially considering the number of different platforms they target. But more than conjecture, we can actually look at their source and know for sure.

OTOH, MSVC is pretty much tied to its CRT, and it may be doing much more assumptions about the CRT implementation than GCC is doing. Which we can't know since we don't have its source.
Frankly I suspect we'd be perfectly fine with 4/8 on Windows. We're worrying about a potential future bug that probably doesn't exist. I tried to bring this up at some point (IRC maybe? I don't remember) and it didn't lead anywhere and I don't care enough to push on it further.
Eric's try results seem to indicate there is no RSS regressions from the change to 8/16, so I'd rather go with security. OTOH, our memory testing on Windows is fairly limited :( I wish we had AWSY for Windows.
(In reply to Mike Hommey [:glandium] from comment #19)
> Eric's try results seem to indicate there is no RSS regressions from the
> change to 8/16, so I'd rather go with security. OTOH, our memory testing on
> Windows is fairly limited :( I wish we had AWSY for Windows.

Can we approximate by testing 8/16 on Linux?
(In reply to David Major [:dmajor] from comment #20)
> (In reply to Mike Hommey [:glandium] from comment #19)
> > Eric's try results seem to indicate there is no RSS regressions from the
> > change to 8/16, so I'd rather go with security. OTOH, our memory testing on
> > Windows is fairly limited :( I wish we had AWSY for Windows.
> 
> Can we approximate by testing 8/16 on Linux?

It won't be a perfect comparison, but I can certainly do a run.
https://areweslimyet.com/?series=erahm_bug_1181142&evenspacing

Some looks better, some looks worse, roughly it's all in the noise. The RSS after TP5 is probably the most interesting, we see a 18.68MiB increase (~5%), but after 30 seconds we drop back down and only see a ~1% increase.

Also note this was a 64-bit build (that's all AWSY supports), I'd expect less of an effect on a 32-bit build. Given the relatively clean talos run it seems like doing the change as-is would probably be fine.
Crash Signature: [@ strncpy | js::DecompileValueGenerator(JSContext*, int, JS::Handle<T>, JS::Handle<T>, int)] → [@ strncpy | js::DecompileValueGenerator(JSContext*, int, JS::Handle<T>, JS::Handle<T>, int)] [@ strncpy | js::DecompileValueGenerator]
This signature is fairly high (#23 currently) in Top Crashers for Firefox 45.0.
What are the next steps here?
Flags: needinfo?(erahm)
(In reply to Mats Palmgren (:mats) from comment #24)
> This signature is fairly high (#23 currently) in Top Crashers for Firefox
> 45.0.
> What are the next steps here?

I think it's reasonable to land this as-is, at this point I'm not inclined to pursue a jemalloc4 upstream follow up. Maybe we can split that off to a separate bug and get this landed?

:glandium, do you agree?
Flags: needinfo?(erahm) → needinfo?(mh+mozilla)
(In reply to Eric Rahm [:erahm] from comment #25)
> (In reply to Mats Palmgren (:mats) from comment #24)
> > This signature is fairly high (#23 currently) in Top Crashers for Firefox
> > 45.0.
> > What are the next steps here?
> 
> I think it's reasonable to land this as-is, at this point I'm not inclined
> to pursue a jemalloc4 upstream follow up. Maybe we can split that off to a
> separate bug and get this landed?
> 
> :glandium, do you agree?

jemalloc4 actually now has a default lg-tiny-min of 3 on all platforms, and it has a configure flag to change it. So if we want to do the same on both, we can either make mozjemalloc use the same value on both win32 and win64, or make jemalloc4 use 4 on win64.
Flags: needinfo?(mh+mozilla)
Bug 691003 made the minimum allocation size word sized for Linux and OS X, we
now need to do a similar change on Windows as well. For Windows the requirement
is 8-bytes on 32-bit and 16-bytes on 64-bit.

It's not clear to me how we would conditionally set the 'lg-tiny-min' flag to 4
for Win64 in jemalloc4 so I've left that out.
Attachment #8734028 - Flags: review?(mh+mozilla)
Attachment #8630810 - Attachment is obsolete: true
Attachment #8734028 - Flags: review?(mh+mozilla) → review+
(In reply to Eric Rahm [:erahm] from comment #27)
> It's not clear to me how we would conditionally set the 'lg-tiny-min' flag
> to 4
> for Win64 in jemalloc4 so I've left that out.

Something like:

if test -n "$HAVE_64BIT_BUILD"; then
  ac_configure_args="$ac_configure_args --with-lg-tiny-min=4"
else
  ac_configure_args="$ac_configure_args --with-lg-tiny-min=3"
fi

in build/autoconf/jemalloc.m4
https://hg.mozilla.org/integration/mozilla-inbound/rev/4593c32d5b81c5e2f2cd1332a7e49c3fc81cbcb0
Bug 1181142 - Part 1: Make the minimum allocation size word sized on all platforms. r=glandium
Keywords: leave-open
Attachment #8734028 - Flags: checkin+
Backed out for xpcshell failure in test_dmd.js on Windows 8 x64 debug.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c49c1cfcbc7c

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4593c32d5b81
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=24617197&repo=mozilla-inbound
19:00:31  WARNING -  TEST-UNEXPECTED-FAIL | memory/replace/dmd/test/test_dmd.js | test - [test : 113] full-unsampled1-live - false == true
19:00:31     INFO -  C:/slave/test/build/tests/xpcshell/tests/memory/replace/dmd/test/test_dmd.js:test:113
19:00:31     INFO -  C:/slave/test/build/tests/xpcshell/tests/memory/replace/dmd/test/test_dmd.js:test2:145
19:00:31     INFO -  C:/slave/test/build/tests/xpcshell/tests/memory/replace/dmd/test/test_dmd.js:run_test:155
19:00:31     INFO -  C:\slave\test\build\tests\xpcshell\head.js:_execute_test:528
Flags: needinfo?(erahm)
So the DMD test is upset because the minimum allocation is now greater than 8 on windows 64. DMD is fine, the test is just rather fragile. If I modify the expected results the test will fail on all other builds.

We could possibly update the test to only malloc 16-bytes and higher. I'm not sure the best way forward here, after looking at the test I'm not clear on how it works at all (lots of dummy functions, something about analyze 1 and analyze 2 specified with aNum, etc).

njn, I think you have pending changes for these tests in another bug. Do you think you could take a look at what needs to be changed after that lands to handle the minimum size being 16-bytes on Windows 64?
Flags: needinfo?(erahm) → needinfo?(n.nethercote)
Attached file diff.txt
This is the diff of the expected and actual output. You can see all the percentages are messed up and the 8 byte allocation has 8 bytes of slop instead of 0 (causing all the other calculations to be off).
I'm happy to make the relevant changes in bug 1253512's patches. (The changes in that bug make the required changes for 16 byte allocations much simpler; score one point for getting rid of "sampling".)

I'm planning to land those patches on Tuesday (Australian time); I won't get to it before then because of the Easter long weekend here. Sorry about that; hopefully a few extra days won't matter in the long run.
Flags: needinfo?(n.nethercote)
Attachment #8734558 - Flags: review?(mh+mozilla) → review+
(In reply to Nicholas Nethercote [:njn] from comment #35)
> I'm happy to make the relevant changes in bug 1253512's patches. (The
> changes in that bug make the required changes for 16 byte allocations much
> simpler; score one point for getting rid of "sampling".)
> 
> I'm planning to land those patches on Tuesday (Australian time); I won't get
> to it before then because of the Easter long weekend here. Sorry about that;
> hopefully a few extra days won't matter in the long run.

Thank you, that should be fine. I think we just want to land it before the next uplift on April 18th.
I'm not especially familiar with mozjemalloc, so this might be totally off-base, but is there a reason you can't just say `#define TINY_MIN_2POW alignof(max_align_t)` now? (This applies to OS X/Linux as well.) That should guarantee the minimum alignment is set properly on all platforms.

I imagine the reason no one used alignof in bug 691003 is that C11 hadn't been ratified by the ISO at the time it landed.
(In reply to Jim Porter (:squib) from comment #38)
> I'm not especially familiar with mozjemalloc, so this might be totally
> off-base, but is there a reason you can't just say `#define TINY_MIN_2POW
> alignof(max_align_t)` now? (This applies to OS X/Linux as well.) That should
> guarantee the minimum alignment is set properly on all platforms.

I suppose you'd have to take the base-2 log of this value, actually. That's probably moderately annoying to do at compile time in C...
We don't have alignof support in all supported compilers.  https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code  But mfbt/TemplateLib.h would have CeilingLog2<N>::value for the purpose, if alignof existed.  (Although of course there's about zero chance of that being upstreamable unless jemalloc becomes C++.)
There's a dev-platform thread about switching to MSVC 2015, and the thread about deprecating OS X 10.6-8 says we're doing 10.6 builds with a dev version of clang 3.8, so perhaps we're close to being able to use alignof? If my reading is accurate, perhaps a followup bug to use alignof would be in order?
(In reply to Jim Porter (:squib) from comment #41)
> There's a dev-platform thread about switching to MSVC 2015, and the thread
> about deprecating OS X 10.6-8 says we're doing 10.6 builds with a dev
> version of clang 3.8, so perhaps we're close to being able to use alignof?
> If my reading is accurate, perhaps a followup bug to use alignof would be in
> order?

Feel free to file a follow up. It looks like |alignof| has been around since MSVC 2012 so we probably don't have to wait for the 2015 switch.

We'll still want to do something that degrades reasonably on non-tier-1 platforms that may not support alignof (or std::max_align_t). For upstream jemalloc (jemalloc 4 as of this writing) they use a config flag rather than a define, so it's not really worth discussing the upstreamability. We'll also need to measure potential regressions on Linux/OSX with such a change (most likely via Talos).

Also note for OSX, in theory, the minimum size is 16-bytes on both 32-bit and 64-bit targets. I'm not sure what |alignof(max_align_t)| would end up being on those platforms, nor am I sure if we'd want to take any regressions without a compelling reason to change (such as crashes).
Well, if there are cases where we don't want to use `alignof(max_align_t)`, I think it'd be reasonable to add those in, provided we fall back to using alignof as a default. (And possibly allow using a config option for platforms that don't support alignof.) I'll file a followup in a moment.
Note the smallest version of clang we support building with is still 3.3, and the smallest version we actually build with on automation is 3.5.

But anyways, is using `alignof(max_align_t)` really worth doing? What we have after this bug landed is close enough, and doesn't need to go through hoops to get the log2 of its result at compile time (and compiling jemalloc.c as C++ *is* going through hoops)
Having looked at the rest of the source file, it looks like it would be easy to just change TINY_MIN_2POW to be the alignment size rather than log2 of the alignment size. All but one instance of TINY_MIN_2POW uses it with bit-shifting operators, and they could be replaced with multiplication or division; an optimizing compiler should preserve performance in those cases.

Here's the only exception I see: <https://dxr.mozilla.org/mozilla-central/source/memory/mozjemalloc/jemalloc.c#1130>.
erahm, I will assume that you will land this patch along with the other ones,
and therefore I don't need to do anything more here. Please let me know if
that's not the case. Thank you.
Attachment #8735718 - Flags: review?(erahm)
Assignee: erahm → n.nethercote
Assignee: n.nethercote → erahm
Comment on attachment 8735718 [details] [diff] [review]
Update DMD tests to handle a minimum alloc size of 16 on Win64

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

r=me Thank you for updating the tests. I'll land this along with the other two patches.
Attachment #8735718 - Flags: review?(erahm) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b78203d8b153d16708b6488f30f131bbe8c8f0
Bug 1181142 - Part 0: Update DMD tests to handle a minimum alloc size of 16 on Win64. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/45c01e7fbe3fb034e0a2b5bfb6a6a745b19497ff
Bug 1181142 - Part 1: Make the minimum allocation size word sized on all platforms. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a14d675236ec1f37d1251b7e9ea5f9aa2aea35e
Bug 1181142 - Part 2: Make the minimum jemalloc4 allocation size 16 bytes on Windows 64. r=glandium
Keywords: leave-open
Depends on: 1261226
This crash is #19 in top-crash for 45.0.2 and #25 for beta.
It increased by ~36% in 45.0.2.
Should we uplift the patch in 47 ?
Flags: needinfo?(erahm)
Keywords: topcrash-win
Just to be clear, 47 is beta now right?

I'll probably have to rebase part 0 (there was some churn in DMD outside of this bug), I imagine part 1 will apply cleanly. We can skip 2.
Flags: needinfo?(erahm)
This is the roll-up for beta if we want to take it. Try run is pending.
yep 47 is beta
Comment on attachment 8745491 [details] [diff] [review]
Make the minimum allocation size word sized on all platforms

Note: This patch is a rebased roll-up of Part 0 and Part 1.

Approval Request Comment
[Feature/regressing bug #]: None.
[User impact if declined]: Top crasher keeps crashing.
[Describe test coverage new/current, TreeHerder]: Baked on nightly, now aurora.
[Risks and why]: None.
[String/UUID change made/needed]: None.
Attachment #8745491 - Flags: approval-mozilla-beta?
Comment on attachment 8745491 [details] [diff] [review]
Make the minimum allocation size word sized on all platforms

Crash fix, there was no crash data for Fx48 that I could use to verify, it's still worth uplifting. Beta47+
Attachment #8745491 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Okay it looks like I need to update the DMD test further, lets hold off on uplifting.
Hi Eric, any updates on when we are ready to uplift this to Beta? Thanks!
Flags: needinfo?(erahm)
(In reply to Ritu Kothari (:ritu) from comment #59)
> Hi Eric, any updates on when we are ready to uplift this to Beta? Thanks!

I'll see what I can do today, I need to figure out how to run the tests locally on the beta branch.
Flags: needinfo?(erahm)
Bug 691003 made the minimum allocation size word sized for Linux and OS X, we
now need to do a similar change on Windows as well. For Windows the requirement
is 8-bytes on 32-bit and 16-bytes on 64-bit.
Attachment #8745491 - Attachment is obsolete: true
Comment on attachment 8749777 [details] [diff] [review]
Make the minimum allocation size word sized on all platforms

This updates the tests to pass with the latest code. The previous version was already approved. 

See comment 57, comment 56. Ritu can you approve this again, we should be good to land now.
Flags: needinfo?(rkothari)
Attachment #8749777 - Flags: approval-mozilla-beta?
Comment on attachment 8749777 [details] [diff] [review]
Make the minimum allocation size word sized on all platforms

Crash fix, Beta47+
Flags: needinfo?(rkothari)
Attachment #8749777 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.