Closed Bug 913883 Opened 11 years ago Closed 11 years ago

Invalid read of size 4 [@ dosprintf]

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED INVALID

People

(Reporter: gkw, Unassigned)

References

Details

(Keywords: regression, testcase, valgrind)

Attachments

(1 file)

Attached file Valgrind stack
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>
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)
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)
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)
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?
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
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 → ---
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.
Pushed --enable-optimize=-O1 into rev 54132426746f when compiling Valgrind builds.
(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 ago11 years ago
Flags: needinfo?(terrence)
Resolution: --- → INVALID
Does that mean we can stop passing --partial-loads-ok=yes to Valgrind?
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.