[NaCl SDK] nacl_io: Fix use-after-free bug in html5fs

[NaCl SDK] nacl_io: Fix use-after-free bug in html5fs

nacl_io::Path::Part returns a temporary string. The code that hashes the path
to create a phony ino calls this, and stashes a pointer to the memory.

The real issue with nacl_io_demo is that the quota was too low. I've upped it
to 5 megs now.

BUG=478230
R=sbc@chromium.org

Committed: https://crrev.com/ccbe99a8e0ab02e923af802bfef76c56fcb803d2
Cr-Commit-Position: refs/heads/master@{#326850}

[Sam Clegg, 2015-04-17 20:52:44]

lgtm.. is there a better long term solution here?  I guess its because we are
doing a lot of mixing of C++ and C strings there, right?   Overhauling Path at
some point in the future might be a good idea.

We really should get that msan bot working..

[khim, 2015-04-17 21:20:05]

khim@chromium.org changed reviewers:
+ khim@chromium.org

[khim, 2015-04-17 21:20:06]

https://codereview.chromium.org/1062463004/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/devfs/dev_fs.cc (right):

https://codereview.chromium.org/1062463004/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/devfs/dev_fs.cc:251: std::string
path_str(path.Join());
Fly-by comment: why "std:sttring" here? Why not "const std::string&" ?

I guess difference in speed shouldn't be large here, but still... it's probably
better explains what you are trying to achieve here.

[binji, 2015-04-18 00:10:21]

OK, I'm wrong about something here. This construct should be legal:
http://stackoverflow.com/questions/1837092/c-destruction-of-temporary-object-...

But I this change did seem to fix the bug... I'm going to take a closer look to
see if the memory is persisted after the function call.

[khim, 2015-04-18 00:27:23]

On Sat, Apr 18, 2015 at 3:10 AM, <binji@chromium.org> wrote:

> OK, I'm wrong about something here. This construct should be legal:
>
>
http://stackoverflow.com/questions/1837092/c-destruction-of-temporary-object-...
>
> But I this change did seem to fix the bug... I'm going to take a closer
> look to
> see if the memory is persisted after the function call.
>
> Memory could be freed here, for example:

const char *ptr = path.Part(segment).c_str();
size_t len = path.Part(segment).length();
hash = HashPathSegment(hash, ptr, len);

Temporary object could be easily destroyed after first line. But where
c_str() is passed into another functions everything should work and if you
want to keep object around then it's better to assign it to "const
std::string&" reference (then temporary object will survive till the end of
function).

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[binji, 2015-04-20 22:35:20]

On 2015/04/18 00:27:23, khim wrote:
> On Sat, Apr 18, 2015 at 3:10 AM, <mailto:binji@chromium.org> wrote:
> 
> > OK, I'm wrong about something here. This construct should be legal:
> >
> >
>
http://stackoverflow.com/questions/1837092/c-destruction-of-temporary-object-...
> >
> > But I this change did seem to fix the bug... I'm going to take a closer
> > look to
> > see if the memory is persisted after the function call.
> >
> > Memory could be freed here, for example:
> 
> const char *ptr = path.Part(segment).c_str();
> size_t len = path.Part(segment).length();
> hash = HashPathSegment(hash, ptr, len);
> 
> Temporary object could be easily destroyed after first line. But where
> c_str() is passed into another functions everything should work and if you
> want to keep object around then it's better to assign it to "const
> std::string&" reference (then temporary object will survive till the end of
> function).

Thanks, it turns out the actual issue was quota, not std::string stuff. But
the one you found here is a real bug too. :)

> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[binji, 2015-04-20 22:35:28]

https://codereview.chromium.org/1062463004/diff/1/native_client_sdk/src/libra...
File native_client_sdk/src/libraries/nacl_io/devfs/dev_fs.cc (right):

https://codereview.chromium.org/1062463004/diff/1/native_client_sdk/src/libra...
native_client_sdk/src/libraries/nacl_io/devfs/dev_fs.cc:251: std::string
path_str(path.Join());
On 2015/04/17 21:20:05, khim wrote:
> Fly-by comment: why "std:sttring" here? Why not "const std::string&" ?
> 
> I guess difference in speed shouldn't be large here, but still... it's
probably
> better explains what you are trying to achieve here.

Done.

[Sam Clegg, 2015-04-20 22:43:34]

lgtm, although might be worth splitting into two CLs

[Sam Clegg, 2015-04-20 22:44:23]

On 2015/04/20 22:43:34, Sam Clegg wrote:
> lgtm, although might be worth splitting into two CLs

Also, /bugs/bug/ in first line of CL description.

[Sam Clegg, 2015-04-24 17:50:25]

On 2015/04/20 22:44:23, Sam Clegg wrote:
> On 2015/04/20 22:43:34, Sam Clegg wrote:
> > lgtm, although might be worth splitting into two CLs
> 
> Also, /bugs/bug/ in first line of CL description.

I suggest we land this despite the windows bots been red (known issue)

[binji, 2015-04-24 18:15:08]

On 2015/04/24 17:50:25, Sam Clegg wrote:
> On 2015/04/20 22:44:23, Sam Clegg wrote:
> > On 2015/04/20 22:43:34, Sam Clegg wrote:
> > > lgtm, although might be worth splitting into two CLs
> > 
> > Also, /bugs/bug/ in first line of CL description.
> 
> I suggest we land this despite the windows bots been red (known issue)

Oops, forgot I haven't landed this yet. :)

[binji, 2015-04-24 18:15:12]

The CQ bit was checked by binji@chromium.org

[commit-bot: I haz the power, 2015-04-24 18:16:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1062463004/40001

[commit-bot: I haz the power, 2015-04-24 18:58:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-24 19:00:08]

Patchset 3 (id:??) landed as
https://crrev.com/ccbe99a8e0ab02e923af802bfef76c56fcb803d2
Cr-Commit-Position: refs/heads/master@{#326850}