Skip to content

Conversation

@smcv
Copy link
Collaborator

@smcv smcv commented Apr 23, 2018

This shouldn't matter unless someone wants to run an inadvisably-named
executable, but it's best-practice for commands that pass on some
of their arguments to a subsequent command.

It allows an invocation like:

bwrap --ro-bind /container / -- "$@"

to search PATH in the container for an executable named according to
"$1", even if $1 has a pathological value like
"--this-has-a-stupid-name--", or even a value that might be
deliberately trying to break bwrap's parsing like "--bind".


(Commit message edited to reflect @AltGr's concerns on #259)

@giuseppe
Copy link
Member

LGTM

Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor comments, also is fine as is.

PATH="${srcd}:$PATH" $RUN -- sh -c "echo hello" > stdout
assert_file_has_content stdout hello
echo "ok - we can run with --"
PATH="${srcd}:$PATH" $RUN -- --inadvisable-executable-name-- > stdout
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also test that passing say --chdir /enoent or something isn't parsed by bwrap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -0,0 +1,2 @@
#!/bin/sh
echo hello
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about generating this dynamically rather than shipping it in the source to be nice to other tools that might process source code and not be prepared to handle such a filename? (Some license checker, sloccount, etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

@AltGr
Copy link

AltGr commented Apr 23, 2018

Thanks!

This shouldn't matter unless someone wants to run an inadvisably-named
executable, but it's best-practice for commands that pass on some
of their arguments to a subsequent command.

It allows an invocation like:

    bwrap --ro-bind /container / -- "$@"

to search PATH in the container for an executable named according to
"$1", even if $1 has a pathological value like
"--this-has-a-stupid-name--", or even a value that might be
deliberately trying to break bwrap's parsing like "--bind".

Fixes: containers#259
Signed-off-by: Simon McVittie <smcv@collabora.com>
@smcv smcv changed the title Add "--" pseudo-argument, for completeness Add "--" pseudo-argument to end option parsing Apr 23, 2018
@smcv
Copy link
Collaborator Author

smcv commented Apr 23, 2018

In addition to responding to @cgwalters' review, I edited the commit message to reflect what @AltGr said on #259 - this is not just for completeness, but also for correctness when wrapping arbitrary commands.

@cgwalters
Copy link
Collaborator

@rh-atomic-bot r+ 848332d

@rh-atomic-bot
Copy link

⌛ Testing commit 848332d with merge fbee75d...

@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: cgwalters
Pushing fbee75d to master...

@smcv smcv deleted the dashdash branch May 14, 2021 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants