Last Comment Bug 788701 - Invalid read of size 2 [@ str_contains] involving map
: Invalid read of size 2 [@ str_contains] involving map
Status: VERIFIED FIXED
[qa-]
: csectype-bounds, sec-high, testcase, valgrind
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla18
Assigned To: :Benjamin Peterson
: general
Mentors:
Depends on:
Blocks: jsfunfuzz
  Show dependency treegraph
 
Reported: 2012-09-05 13:42 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2012-12-13 17:06 PST (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed
unaffected


Attachments
decrease length appropriately (941 bytes, patch)
2012-09-10 10:08 PDT, :Benjamin Peterson
jorendorff: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-09-05 13:42:42 PDT
[,,8].map(String.prototype.contains,this)

shows the following error on Valgrind in 32-bit js opt shell on m-c changeset ce16c39e01fb:

==463== Invalid read of size 2
==463==    at 0xE30FB: str_contains(JSContext*, unsigned int, JS::Value*) (jsstr.cpp:984)
==463==    by 0x8E0BF: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:372)
==463==    by 0x2A235: array_map(JSContext*, unsigned int, JS::Value*) (jsinterp.h:119)
==463==    by 0x8E0BF: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:372)
==463==    by 0x860E4: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2405)
==463==    by 0x7ADAB: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:301)
==463==    by 0x8E870: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:486)
==463==    by 0x8E9E2: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:523)
==463==    by 0x1EC1C: JS_ExecuteScript (jsapi.cpp:5665)
==463==    by 0x74CA: Process(JSContext*, JSObject*, char const*, bool) (js.cpp:433)
==463==    by 0x6007: Shell(JSContext*, js::cli::OptionParser*, char**) (js.cpp:4738)
==463==    by 0x681C: main (js.cpp:4950)
==463==  Address 0x2749070 is 0 bytes after a block of size 32 alloc'd
==463==    at 0x3D3785: malloc (vg_replace_malloc.c:274)
==463==    by 0x1B0586: js::Vector<unsigned short, 32ul, js::ContextAllocPolicy>::extractRawBuffer() (Utility.h:153)
==463==    by 0x1B020E: js::StringBuffer::finishString() (StringBuffer.cpp:21)
==463==    by 0x9CAEA: js::obj_toStringHelper(JSContext*, JSObject*) (jsobj.cpp:302)
==463==    by 0xA246A: obj_toString(JSContext*, unsigned int, JS::Value*) (jsobj.cpp:359)
==463==    by 0x8E0BF: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:372)
==463==    by 0x8E346: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:119)
==463==    by 0xAD1BF: js::DefaultValue(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) (jsobj.cpp:4897)
==463==    by 0xE7F21: js::ToStringSlow(JSContext*, JS::Value const&) (jsobjinlines.h:69)
==463==    by 0xE2E0C: str_contains(JSContext*, unsigned int, JS::Value*) (jsapi.h:1467)
==463==    by 0x8E0BF: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:372)
==463==    by 0x2A235: array_map(JSContext*, unsigned int, JS::Value*) (jsinterp.h:119)


Command:

valgrind --dsymutil=yes --leak-check=full --suppressions=/Users/fuzz3/fuzzing/known/mozilla-central/valgrind.txt ./js testcase.js


May be related to bug 779025. s-s because this is an invalid read.
Comment 1 Jason Orendorff [:jorendorff] 2012-09-05 18:23:07 PDT
Definitely not related. That's a different kind of Map.
Comment 2 Jesse Ruderman 2012-09-05 19:57:21 PDT
autoBisect.py --valgrind -a32 -p 788701.js
(with some autoBisect tweaks)

The first bad revision is:
changeset:   87e7abe891a9
user:        Benjamin Peterson
date:        Fri Aug 03 11:37:27 2012 -0700
summary:     Bug 772733 - Implement String.contains. r=sfink
Comment 3 :Benjamin Peterson 2012-09-05 20:15:23 PDT
Could reproduce on 64-bits.
Comment 4 :Benjamin Peterson 2012-09-05 20:15:40 PDT
(In reply to Benjamin Peterson [:benjamin] from comment #3)
> Could reproduce on 64-bits.

Uh, rather, could not reproduce.
Comment 5 Jesse Ruderman 2012-09-05 20:41:56 PDT
That was my experience as well.  Let me know if you need help building 32-bit.
Comment 6 Jason Orendorff [:jorendorff] 2012-09-08 07:31:24 PDT
I need help. Is it possible to build 32-bit on Mac these days? What's documented doesn't work, because the current code no longer compiles under gcc-apple-4.2.

https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions/Mac_OS_X_Prerequisites
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2012-09-08 11:46:39 PDT
I append environment variables - tested on 10.7 and 10.8.

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -fcolor-diagnostics -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -fcolor-diagnostics -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -fcolor-diagnostics" HOST_CXX="clang++ -Qunused-arguments -fcolor-diagnostics" sh ./configure --target=i386-apple-darwin8.0.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --enable-more-deterministic --disable-tests --enable-valgrind

(Switch to Clang?)
Comment 8 Jason Orendorff [:jorendorff] 2012-09-10 05:44:31 PDT
I switched to Clang a while ago. I guess we should switch the docs too.

OK, now the problem I have is that valgrind won't run the resulting executable.
Comment 9 Gary Kwong [:gkw] [:nth10sd] 2012-09-10 05:46:45 PDT
> OK, now the problem I have is that valgrind won't run the resulting
> executable.

Try compiling it manually, docs are at http://www.valgrind.org/downloads/repository.html
Comment 10 Christian Holler (:decoder) 2012-09-10 06:59:51 PDT
Or simply use Linux, the bug is not Mac specified.
Comment 11 Christian Holler (:decoder) 2012-09-10 07:55:23 PDT
I quickly looked into this in a debug build, and it seems that StringMatch is being called with a text pointer that is inside the "[Object global]" string, rather than at the start:

Breakpoint 1, StringMatch (text=0x8600114, textlen=15, pat=0xf7500328, patlen=1) at js/src/jsstr.cpp:971
971         if (patlen == 0)
(gdb) x /30bx 0x8600114
0x8600114:      0x62    0x00    0x6a    0x00    0x65    0x00    0x63    0x00
0x860011c:      0x74    0x00    0x20    0x00    0x67    0x00    0x6c    0x00
0x8600124:      0x6f    0x00    0x62    0x00    0x61    0x00    0x6c    0x00
0x860012c:      0x5d    0x00    0x00    0x00    0xa0    0x52

The string there reads "bject global]" + some junk because textlen is still 15. The pointer is off by n*sizeof(jschar) where n is the number of undefined objects before the "8" in the array. If you add more undefined elements before 8, then the pointer goes even further off.

Maybe this helps to find the bug :)
Comment 12 :Benjamin Peterson 2012-09-10 10:08:52 PDT
Created attachment 659757 [details] [diff] [review]
decrease length appropriately

Stupid mistake in my initial implementation.
Comment 13 Jason Orendorff [:jorendorff] 2012-09-13 09:35:28 PDT
Comment on attachment 659757 [details] [diff] [review]
decrease length appropriately

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

Thanks again, Benjamin!

Please add two tests in string-contains.js (or wherever):

assertEq("xyzzy".contains("zy\0", 2), false);

var dots = Array(1000000).join('.');
assertEq(dots.contains("\x01", 1000000), false);
assertEq(dots.contains("\0", 1000000), false);

These should more reliably detect the bug (with a wrong answer or a crash, not just a valgrind warning).

r=me with that.
Comment 14 :Benjamin Peterson 2012-09-13 09:43:30 PDT
Comment on attachment 659757 [details] [diff] [review]
decrease length appropriately

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 772733
User impact if declined: JS reads memory which doesn't belong to it
Testing completed (on m-c, etc.): Green try
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-09-13 19:02:02 PDT
https://hg.mozilla.org/mozilla-central/rev/701e901721bf
Comment 18 Gary Kwong [:gkw] [:nth10sd] 2012-12-13 17:06:31 PST
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.

Note You need to log in before you can comment on or make changes to this bug.