Page MenuHomeFreeBSD

Allow rtld direct-exec to take a file descriptor.
ClosedPublic

Authored by jonathan on May 15 2017, 11:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 7:39 AM
Unknown Object (File)
Thu, Oct 31, 4:58 AM
Unknown Object (File)
Oct 25 2024, 4:19 PM
Unknown Object (File)
Oct 23 2024, 7:00 PM
Unknown Object (File)
Oct 18 2024, 1:09 AM
Unknown Object (File)
Oct 17 2024, 10:12 PM
Unknown Object (File)
Oct 5 2024, 2:49 AM
Unknown Object (File)
Oct 3 2024, 12:57 PM
Subscribers

Details

Summary

When executing rtld directly, allow the value of argv[0] to specify a
file descriptor of the binary to execute. If this fails, fall back to
the previous behaviour of treating argv[0] as a path.

Given the newfound ability of rtld to be directly executed, this approach can be
used as an alternative to creating an ffexecve(2) system call.

This diff has two commits in it: one to rename parse_libdir->parse_integer
and the other to actually do the exec-an-fd work.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9343
Build 9806: arc lint + arc unit

Event Timeline

I do not like these argv0 tricks, I intend to implement normal options parsing for the direct mode. One of the options would take the file descriptor number and do what your trick does. I think that this is better than the trick since it allows to easily invoke and test the functionality from the shell.

That said, please commit the rename of parse_libdir now. This is the change which is definitely needed for the options parser.

libexec/rtld-elf/rtld.c
5298

This message is lost. Wouldn't it make sense to put it in the dirfd < 0 action above ?

In D10751#222729, @kib wrote:

I do not like these argv0 tricks, I intend to implement normal options parsing for the direct mode. One of the options would take the file descriptor number and do what your trick does. I think that this is better than the trick since it allows to easily invoke and test the functionality from the shell.

You're right, that would be better. Were you thinking of getopt-style parsing (but without pulling in getopt) or environment variables (e.g., LD_BINARY_FD=xx)? The environment variable approach has the benefit of not requiring any new parsing code, but an explicit --fd xx will look more familiar. Then again, if we provide options people might come to expect --help and --usage options. :)

That said, please commit the rename of parse_libdir now. This is the change which is definitely needed for the options parser.

Ok, I'll do that this morning (currently building from a clean checkout, then I'll run the LD_LIBRARY_PATH_FDS test again).

libexec/rtld-elf/rtld.c
5298

Yes, that would make sense... I'll update the review once I've committed the parse_integer change.

Were you thinking of getopt-style parsing (but without pulling in getopt) or environment variables (e.g., LD_BINARY_FD=xx)? The environment variable approach has the benefit of not requiring any new parsing code, but an explicit --fd xx will look more familiar. Then again, if we provide options people might come to expect --help and --usage options. :)

I intended to provide syntax like

ld-elf.so.1 [-ph] [-f fd] [--] binary args

-h outputs short usage summary, -p means search the $PATH paths for binary, -f fd starts binary pointed to by the filedescriptor fd, -- ends the options list.

Yes, I do not want to reference getopt(3) routine from libc, it would not work anyway, but very similar code is due.

For environment variables I agree that this is not very convenient, but my other concern is that somebody must clean the environment for the started image, since LD_BINARY_FD does not make a sense after ld.so acted, and even is detrimental. ld.so can do the censorship, but I prefer to keep the environment intact.

  • Add more explicit argument parsing to rtld.
  • Merge remote-tracking branch 'origin/master' into arcpatch-D10751
  • Remove a stray (debug) utrace(2) invocation.
jonathan marked 2 inline comments as done.
  • Apply a style(9) fix.

Two small notes, this is not a review. Mostly to inform you about D10750.

libexec/rtld-elf/rtld.c
435

{} are not needed

5328

For this, see D10750.

  • Merge 'origin/master' into arcpatch-D10751
  • Merge 'origin/master' into arcpatch-D10751

This merge pulls in the changes from D10750, dropping the now-redundant checks on executable permission bits.

  • Drop unnecessary defensive braces.
  • Fix argv/environ/aux shift calculations.
libexec/rtld-elf/rtld.c
5299

This is bug, --help is matched as well.

5312

This is bug, -fdull <n> is matched as well.

And, why do you want -fd be multi-char option, while other options are single-char ? -f <n> works fine, IMO.

5324

I do not see why this check is needed. I understand why do you want to make sure that fd is a file descriptor at all, ie. the fstat(2) call above makes sense. But properties of the fd are not important, as far at it quacks like a suitable duck.

5336

Why do you use strn* functions ? Their use for anything but d_name in V7 struct dirent is plain bug, like all uses in the patch.

libexec/rtld-elf/rtld.c
5299

Ok, I missed that the length is 3, so the nul symbol participates. But then use of strncmp is even less sensible.

jonathan marked 4 inline comments as done.
  • Address comments in kib's review.
jonathan marked an inline comment as done.EditedMay 17 2017, 4:36 PM

Ok, I think I've addressed all of these points now. I suppose that my use of strncmp comes from... my fingers just don't want to type functions that start with strc? :) Anyhow, I've changed all of the strncmp to strcmp and strnlen to strlen as you've asked.

Overall this looks good.

libexec/rtld-elf/rtld.c
5324

Excessive () around arglen - 1 expression.

5339

In fact this stat is not needed, because caller does stat anyway, to check permissions. But the error message at that stat call failure probably needs adjustments when we stating not opened argv0 but fd.

jonathan marked 2 inline comments as done.
  • Remove some superfluous parentheses.
  • Remove redundant fstat(2) call.
libexec/rtld-elf/rtld.c
5339

Good point... how does the new message ("failed to FD <fd> for <path>") look? It can either mean "failed to stat this FD that I opened for <path>" or "failed to stat this FD that you gave me for <path>"... I think maybe it works either way?

libexec/rtld-elf/rtld.c
5339

It might be less efforts to formulate something sound, and less confusing to user, if you create a boolean to track the source of fd, was it opened or obtained as the argument. Set it together with open() call.

Then you can put specific message about fstat. Arguably strerror(errno) should be included into the message.

libexec/rtld-elf/rtld.c
5339

Sure, I can do that.

Also, it looks like I've been using tabs for all levels of indent rather than four spaces for second-level indents... I'll need to fix that too.

  • Add a missing rtld_strerror()
  • Convert some _rtld_error() calls to rtld_printf().
  • Use four spaces for second-level indent.
  • Be clear about user- vs path-derived descriptor.

For new functions in rtld, I use normal style(9), so rtld slowly migrates to proper indentation. For smaller changes to existing functions I do follow existing style of 4 spaces indent/2 spaces continuation.

This revision is now accepted and ready to land.May 17 2017, 9:37 PM
In D10751#223329, @kib wrote:

For new functions in rtld, I use normal style(9), so rtld slowly migrates to proper indentation. For smaller changes to existing functions I do follow existing style of 4 spaces indent/2 spaces continuation.

Ah, I see... then I suppose I should put the tabs back before committing.

jonathan edited edge metadata.
  • Restore tabs to print_args().
This revision now requires review to proceed.May 17 2017, 10:35 PM
This revision was automatically updated to reflect the committed changes.