Make nacl_helper_bootstrap load a PIE normally, not the dynamic linker directly

Make nacl_helper_bootstrap load a PIE normally, not the dynamic linker directly

This makes it more like the way a PIE is normally run when loaded by
the kernel.  It doesn't rely on the dynamic linker's feature of being
run from the command line.  It also avoids hard-coding the canonical
machine-dependent file name of the dynamic linker.  Instead, it just
obeys PT_INTERP the same way the kernel would.

BUG= none
TEST= nacl still works

R=mseaborn@chromium.org,bradchen@google.com

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109734

[Mark Seaborn, 2011-11-11 19:28:25]

http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
File chrome/nacl/nacl_helper_bootstrap_linux.c (right):

http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
chrome/nacl/nacl_helper_bootstrap_linux.c:266: * Usually PT_INTERP is before the
first PT_LOAD.
Can you just have one loop that looks for PT_INTERP, for simplicity?  i.e. Drop
the earlier one.  Avoiding scanning the phdrs array more than once seems like a
rather unnecessary optimisation.

http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
chrome/nacl/nacl_helper_bootstrap_linux.c:334: *out_interp = (const char *)
(interp->p_vaddr + load_bias);
It's OK to assume that PT_INTERP is covered by a PT_LOAD segment, then?

http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
chrome/nacl/nacl_helper_bootstrap_linux.c:383: * Now move everything back to eat
our original argv[0].  When we've done
Is there a reason why you don't change the stack to point one entry higher, and
overwrite argv[0] with argc?  Does that mess up the command line reported in
/proc?

http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
chrome/nacl/nacl_helper_bootstrap_linux.c:456: asm(".text\n"
Was it wrong without .text before?  Would .pushsection/.popsection be better?

[Roland McGrath, 2011-11-11 21:05:17]

PTAL

On 2011/11/11 19:28:25, Mark Seaborn wrote:
>
http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
> File chrome/nacl/nacl_helper_bootstrap_linux.c (right):
> 
>
http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
> chrome/nacl/nacl_helper_bootstrap_linux.c:266: * Usually PT_INTERP is before
the
> first PT_LOAD.
> Can you just have one loop that looks for PT_INTERP, for simplicity?  i.e.
Drop
> the earlier one.  Avoiding scanning the phdrs array more than once seems like
a
> rather unnecessary optimisation.

Done.

>
http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
> chrome/nacl/nacl_helper_bootstrap_linux.c:334: *out_interp = (const char *)
> (interp->p_vaddr + load_bias);
> It's OK to assume that PT_INTERP is covered by a PT_LOAD segment, then?

It is in practice.  But neither the spec nor the kernel requires it.
So I've made it robust.

>
http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
> chrome/nacl/nacl_helper_bootstrap_linux.c:383: * Now move everything back to
eat
> our original argv[0].  When we've done
> Is there a reason why you don't change the stack to point one entry higher,
and
> overwrite argv[0] with argc?  Does that mess up the command line reported in
> /proc?

It does break that.  It also breaks the ABI stack alignment requirements.

>
http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
> chrome/nacl/nacl_helper_bootstrap_linux.c:456: asm(".text\n"
> Was it wrong without .text before?

It didn't happen to go wrong here, but it was quite fragile.

> Would .pushsection/.popsection be better?

Yes, that's the best thing to use.  I've made it do that now.

[Mark Seaborn, 2011-11-11 23:23:28]

LGTM

On 11 November 2011 13:05, <mcgrathr@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
>
>> chrome/nacl/nacl_helper_bootstrap_linux.c:334: *out_interp = (const char
>> *)
>> (interp->p_vaddr + load_bias);
>> It's OK to assume that PT_INTERP is covered by a PT_LOAD segment, then?
>>
>
>
> It is in practice.  But neither the spec nor the kernel requires it.
>

That's a pity it's not required.


> So I've made it robust.
>

Hmm, with a fallback case that will, in practice, never get run, and that
uses a global buffer. :-(  I assume you tested it manually by setting the
conditional to be false?  My LGTM is with a sigh because I don't like
having fallbacks like this, and I'm sorry I queried this. :-)

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2011-11-11 23:23:31]

LGTM

On 11 November 2011 13:05, <mcgrathr@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/8524015/diff/1/chrome/nacl/nacl_helper_bootstr...
>
>> chrome/nacl/nacl_helper_bootstrap_linux.c:334: *out_interp = (const char
>> *)
>> (interp->p_vaddr + load_bias);
>> It's OK to assume that PT_INTERP is covered by a PT_LOAD segment, then?
>>
>
>
> It is in practice.  But neither the spec nor the kernel requires it.
>

That's a pity it's not required.


> So I've made it robust.
>

Hmm, with a fallback case that will, in practice, never get run, and that
uses a global buffer. :-(  I assume you tested it manually by setting the
conditional to be false?  My LGTM is with a sigh because I don't like
having fallbacks like this, and I'm sorry I queried this. :-)

Mark