Closed
Bug 913883
Opened 11 years ago
Closed 11 years ago
Invalid read of size 4 [@ dosprintf]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
INVALID
People
(Reporter: gkw, Unassigned)
References
Details
(Keywords: regression, testcase, valgrind)
Attachments
(1 file)
(deleted),
text/plain
|
Details |
schedulegc()
shows an invalid read of size 4 on m-c changeset 3697f962bb7b without any CLI arguments.
s-s because this is an invalid read. Tested with `valgrind --smc-check=all-non-file ./js testcase.js`.
My configure flags are:
--enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-valgrind --with-ccache --enable-threadsafe <other NSPR options>
Reporter | ||
Comment 1•11 years ago
|
||
autoBisect shows this is probably related to the following changeset:
The first bad revision is:
changeset: http://hg.mozilla.org/mozilla-central/rev/6acc72608961
user: Terrence Cole
date: Fri Nov 09 09:45:25 2012 -0800
summary: Bug 811060 - Move DeflateString out of jsstr and make it Typey; r=Waldo
Terrence, is bug 811060 a likely regressor?
Blocks: 811060
Flags: needinfo?(terrence)
Comment 2•11 years ago
|
||
Uh oh, bugzilla seems to have eaten the comment I made here last night -- or worse, I'll find it on an unrelated bug in 6 months. Anyway, I'll try to reproduce what I wrote last night, but this may be a bit sparser on details than I'd like.
The line valgrind is complaining about is:
slen = strlen(s);
With -O3, ggc inlines this. The first part of the function looks like:
>|0x5e1d4b <dosprintf(...)+3803> mov (%rdx),%ecx
│0x5e1d4d <dosprintf(...)+3805> add $0x4,%rdx
│0x5e1d51 <dosprintf(...)+3809> lea -0x1010101(%rcx),%eax
│0x5e1d57 <dosprintf(...)+3815> not %ecx
│0x5e1d59 <dosprintf(...)+3817> and %ecx,%eax
│0x5e1d5b <dosprintf(...)+3819> and $0x80808080,%eax
│0x5e1d60 <dosprintf(...)+3824> je 0x5e1d4b <dosprintf(...)+3803
The string s in this case is "schedulegc(num | obj)", a malloced region of 22 bytes, stored in $rdx. The loop above loads 4 bytes at a time from this string. Valgrind complains at the last load in the loop when we get bytes 20 to 24 since the last two bytes in the load are not in the allocated region. After this block the uninitialized bytes get shifted out and do not escape, so this is not a sec-problem: I've unchecked the sec-sensitive field.
|dosprintf| appears to be riddled with inlined strlen that look exactly like this, so I'm wondering why valgrind complains here but not elsewhere. Julian, do you have any insight into why this particular case is problematic?
Group: core-security
Flags: needinfo?(terrence) → needinfo?(jseward)
Comment 3•11 years ago
|
||
You need --partial-loads-ok=yes to tell V that these overruns are acceptable.
See http://valgrind.org/docs/manual/mc-manual.html#mc-manual.options
Also you'd be best advised to use the V trunk, since these inlined strlens
are problematic to handle and the trunk has the best handling of them so
far.
Also .. we never claimed that Memcheck will work well on -O3 code. The
basic recommendation is to stick to -O so as to avoid Memcheck getting
confused by aggressive gcc optimisations.
Flags: needinfo?(jseward)
Comment 4•11 years ago
|
||
Thanks, Julian!
Gary, should we close this as invalid now, or do you want to use it to track the tooling changes that Julian suggested above?
Reporter | ||
Comment 5•11 years ago
|
||
Thanks Terrence & Julian!
(In reply to Julian Seward from comment #3)
> You need --partial-loads-ok=yes to tell V that these overruns are
> acceptable.
> See http://valgrind.org/docs/manual/mc-manual.html#mc-manual.options
Added to the harness in rev 5fc9a72aa3ca.
>
> Also you'd be best advised to use the V trunk, since these inlined strlens
> are problematic to handle and the trunk has the best handling of them so
> far.
>
Yup, I'm using usually the latest on SVN trunk.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
Reporter | ||
Comment 6•11 years ago
|
||
Using valgrind --smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access --partial-loads-ok=yes ./js testcase.js :
schedulegc()
I still get this on rev 53a2011a01b2:
--enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-valgrind --with-ccache --enable-threadsafe <more NSPR options>
fwiw, I'm on Valgrind SVN tip r13572.
Terrence, does the default build options build with -O3 by default? How can I overwrite it to -O on Linux?
Reopening to track this.
Status: RESOLVED → REOPENED
Flags: needinfo?(terrence)
Resolution: INVALID → ---
Reporter | ||
Comment 7•11 years ago
|
||
decoder pointed out to me that I should use --enable-optimize="-O1", so since -O is equal to -O1 ( http://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html ) that's what I'll try out.
Reporter | ||
Comment 8•11 years ago
|
||
Pushed --enable-optimize=-O1 into rev 54132426746f when compiling Valgrind builds.
Reporter | ||
Comment 9•11 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] (PTO Sep 24 - 27) from comment #8)
> Pushed --enable-optimize=-O1 into rev 54132426746f when compiling Valgrind
> builds.
I've verified that the error no longer shows in -O1 Valgrind builds. \o/
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: needinfo?(terrence)
Resolution: --- → INVALID
Comment 10•11 years ago
|
||
Does that mean we can stop passing --partial-loads-ok=yes to Valgrind?
Reporter | ||
Comment 11•11 years ago
|
||
I've removed --partial-loads-ok=yes in fuzzing rev f71794038dbb for now.
You need to log in
before you can comment on or make changes to this bug.
Description
•