913883 - Invalid read of size 4 [@ dosprintf]

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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>

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 2

[Terrence Cole [: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?

Comment 3

[Julian Seward [: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.

Comment 4

[Terrence Cole [:terrence]]

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?

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Pushed --enable-optimize=-O1 into rev 54132426746f when compiling Valgrind builds.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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/

Comment 10

[Jesse Ruderman]

Does that mean we can stop passing --partial-loads-ok=yes to Valgrind?

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I've removed --partial-loads-ok=yes in fuzzing rev f71794038dbb for now.