Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass the elements from `BUILD_PATH_PREFIX_MAP` to the assembler #1930

Merged
merged 19 commits into from Jul 27, 2018

Conversation

Projects
None yet
4 participants
@xclerc
Copy link
Contributor

commented Jul 25, 2018

Given that as does not inspect the BUILD_PATH_PREFIX_MAP
environment variable, it is necessary to pass its elements explicitly.
Otherwise, the produced object files contain the the absolute paths
of their sources, leading to non-reproducible builds.

@gasche
Copy link
Member

left a comment

This looks nice, see comments for a change suggestion.

@@ -96,6 +96,9 @@ val rewrite_absolute_path: string -> string
variable (https://reproducible-builds.org/specs/build-path-prefix-map/)
if it is set. *)

val get_build_path_prefix_map: unit -> Build_path_prefix_map.map option
(** Returns the map used by [rewrite_absolute_path]. *)

This comment has been minimized.

Copy link
@gasche

gasche Jul 25, 2018

Member

I think that this should not be in Location. Could you maybe move the logic in utils/misc.ml, and then use it from Location? (I would rather keep it out of build_path_prefix_map.ml, so that that one remains pure encoding/decoding.)

map;
Buffer.contents buff
end else
""

This comment has been minimized.

Copy link
@gasche

gasche Jul 25, 2018

Member

This shorter case should go first (so if not Config...).

configure Outdated
@@ -1909,6 +1909,12 @@ asm_cfi_supported=false

export as aspp

if $as --debug-prefix-map old=new simple.S; then

This comment has been minimized.

Copy link
@gasche

gasche Jul 25, 2018

Member

I suppose that this will write on stderr in the case where it does not work. Is it the expect behavior, or do other tryfoo.sh tests typically silence the failure output?

This comment has been minimized.

Copy link
@xclerc

xclerc Jul 25, 2018

Author Contributor

One would get a message akin to:

as: unrecognized option '--debug-prefix-map'

I don't think it is a major usability issue, but for the sake
of consistency, I added a redirection.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

@gasche
Copy link
Member

left a comment

Good to merge if the CI passes.

Changes Outdated
@@ -114,6 +114,9 @@ Working version
and expect tests
(Armaël Guéneau, review by Thomas Refis, Gabriel Scherer and François Bobot)

- GPR#????: pass the elements from `BUILD_PATH_PREFIX_MAP` to the assembler
(Xavier Clerc, review by ???)

This comment has been minimized.

Copy link
@gasche

gasche Jul 25, 2018

Member

Remember to update ???.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

And finally for this file: I would actually have written the code with a
fold rather than an iter. Did you consider this possibility?

I am not sure I see what you mean; what would be the type of your
accumulator?

Also: are you sure the feature is not supported on MingW? Well this is
minor, I guess it can always be enabled later.

Well, I guess it might depend on the assembler and/or its version,
but there is no detection logic. I think we are on the safe side by
assuming it is not supported.

Finally: your PR modifies ocamldoc/Makefile.unprefix. Was this change
intended? I'm not sure I understand how it's related to the rest of the
PR?

The patch introduced a dependency from location.cmi to build_path_prefix_map.cmi
that made this change necessary. Given the changes requested by @gasche,
the dependency disappeared and I reverted this change.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

(Please squash when merging; this PR not only has an uninteresting
history but also contains a number of non-buildable intermediary steps.)

xclerc added some commits Jul 25, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

The CI was unhappy because of check-typo issues;
223ed09 fixes these issues. If Travis/AppVeyor agree,
I think this PR is ready to be merged.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

I have basically no opinion; the version of this PR incurs
a closure allocation, while your version incurs a list allocation
(notwithstanding the allocations hidden behind Printf.sprinf).

However, to be fair, I don't think efficiency in this part of the
compiler is really important, as this is going to be executed once
and we are about to run an external command.

@gasche @shindere Which version should be committed?

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

Personally I find @xclerc's version actually a bit more readable, because I can quickly recognize the pattern of printing into a temporary buffer, while it takes me a bit more time to recognize the pattern of building a list of strings to concatenate them. This is strictly based on past experience, not an objective measure of which is better.

On the other hand, @shindere's solution suggests the use of a Printf formatter instead of separated printing calls. Possibly my personal favorite would be using Printf.bprintf buff " --debug-prefix-map %s=%s".

Which makes me realize that there is a question that I should have asked before: what if there are special characters in the prefix map? BUILD_PATH_PREFIX_MAP comes with its own (somewhat baroque) encoding scheme for having eg. " in the paths on either side.

As far as I can tell, there is absolutely no attempt in your version to prevent special characters in the map to destroy the command-line -- this sounds bad to me, as we enter the "SQL injection" territory. At the very least, I would feel safer with something like " --debug-prefix-map "%s=%s", paired with an assert that the two sides don't contain double-quotes (or just silently dropping those that do); a more robust encoding scheme would also be nice.

What is the situation for the other command-line-invocations that we generate today? How much of them can be completely controlled by user inputs, in ways that may result in arbitrary commands being executed? Is there a notion of "safe" encoding for emitted command-lines?

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

@gasche I will have a look at the issue, but it seems unfair to classify
it as SQL-injection-like. If someone can set your environment variables,
you have bigger problems.

(Can you remove your approval, so that the PR is not merged by mistake?)

@gasche gasche dismissed their stale review Jul 26, 2018

we need to think about escaping issues just a bit more carefully

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

I was worried about cases of environment variables flowing into more-privileged execution contexts, but (1) in that case it would require the compiler to be setuid rather than a user program, and that sounds already dangerous without this change and (2) it is possible that the recent secure_getenv changes would mitigate this scenario. So yes, after thinking more about it I'm less worried about the problem, but we both agree that it deserves a bit of thought -- if only to convince ourselves that other command-line generated are problematic in exactly the same way, so that there is no point in specific ad-hoc mitigation for this one.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

I am slightly confused regarding the security aspect; are your concerns
about commands built from elements passed e.g. to the compiler? If so,
the following comes to mind:

ocaml -use-runtime "evil-command" source.ml

or:

ocaml -use-runtime "evil-command ; ./non-evil/ocamlrun" source.ml

Regarding the escaping, I am wondering whether the following would be
enough in practice (notice that contrary to BUILD_PATH_PREFIX_MAP, the
command-line switch from as does not appear to use a fancy encoding):

" --debug-prefix-map %S=%S"
@gasche

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

I think that %S=%S would be reasonable if as properly supports it -- I guess it does. (The specification of %S does not say that its encoding makes command-line parsers interpret the payload as a single argument, and indeed it would be hard to ensure this in a portable way, but this may have to do/suffice.)

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

@gasche
Copy link
Member

left a comment

I think we all agree that simply quoting the arguments is enough for now.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

No, you're quoting arguments wrong. %S quotes in OCaml syntax, while Filename.quote quotes in shell syntax. Please look elsewhere at how external commands are invoked.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

On the general issue of quoting things for execution through a shell, see #1492. For Unix systems, it's enough to Filename.quote every argument, then concatenate with spaces. That's what we do in ocamlc and ocamlopt. For Win32 additional quoting is needed, which #1492 tries to get right.

If/once #1492 gets through, invocations of external tools should use Filename.quote_command, meaning that the command line should be prepared as a list of strings (one element = one argument) and not a single string. ( ocamlbuild fans know this style.) It would be nice to design this PR so that it will be easy to adapt to this style.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

@xavierleroy Does this mean that the Filename.quote should be used here?

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

It would be nice to design this PR so that it will be easy to adapt to this style.

Then that suggests using @shindere's proposed implementation with List.map, which is easier to adapt to build a non-string type of values

@xclerc, in addition to use Filename.quote instead of %S, could you make that change? I believe that the PR should be good to go then.

@gasche gasche dismissed their stale review Jul 26, 2018

(still minor changes to perform before merging)

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

Does this mean that the Filename.quote should be used here?

"here" has nothing to do with this PR. But, yes, "here" should use Filename.quote to harden against spaces and shell special characters in the path to the custom runtime. However, other problems beyond our control will occur if the path to the custom runtime contains spaces, e.g. the #! headers won't work.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

(I was trying to evaluate whether it would be worthwhile to open
another PR to fix quoting in other places.)

@gasche

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

(Also: Xavier should be added as a reviewer.)

@gasche

gasche approved these changes Jul 26, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

This PR seems ready to be merged.

@gasche gasche merged commit 7e29162 into ocaml:trunk Jul 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@xclerc xclerc referenced this pull request Nov 13, 2018

Closed

Reproducible builds #714

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.