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

Don't invoke the bundled protoc binary on non-gnu Linux hosts #202

Closed
mzabaluev opened this issue Jun 28, 2019 · 25 comments · Fixed by #410
Closed

Don't invoke the bundled protoc binary on non-gnu Linux hosts #202

mzabaluev opened this issue Jun 28, 2019 · 25 comments · Fixed by #410

Comments

@mzabaluev
Copy link
Contributor

Revisiting the problem reported in #182. Unfortunately, it percolates to builds that use tower-grpc-build and troubleshooting clues are currently less than ideal.

Is it possible to avoid running the bundled protoc on non-glibc (e.g. musl) Linux host environment and report that a binary needs to be supplied? Notably this is different from the build target.

@mzabaluev mzabaluev changed the title Don't invoke the bundled protoc binary on non-gnu Linux targets Don't invoke the bundled protoc binary on non-gnu Linux hosts Jun 28, 2019
@danburkert
Copy link
Collaborator

Ah, is this coming up when using prost on Alpine Linux? I don't think I fully grasped the significance of the report in #182; I thought it was a really oddball setup that I wouldn't be able to reproduce. It should definitely be possible to hack support for dynamically detecting non-existence of glibc and falling back to the environment or path. See https://github.com/danburkert/prost/blob/master/prost-build/build.rs for where this all happens.

@danburkert
Copy link
Collaborator

Or, for a real challenge, convince upstream to ship fully static binaries :)

@mzabaluev
Copy link
Contributor Author

It should definitely be possible to hack support for dynamically detecting non-existence of glibc

There may be way too many corner cases; this would need to check if the libraries exist in the library path where the /lib*/ld*.so.? loaded by the binary would expect to find them, which is configurable (both in /etc and via ${LD_LIBRARY_PATH}). It may be easier to try to run ldd and parse its output.

I'd rather suggest checking the host tuple (available from ???) or uname -o. Unfortunately, env::consts::OS does not provide detailed information on the flavor of Linux.

@nagisa
Copy link
Contributor

nagisa commented Jul 25, 2019

I’m still somewhat confused why asking users to make protoc available in the environment and using it by default is an issue. Its a build too like any other.

@cpcloud
Copy link

cpcloud commented Oct 4, 2019

I second @nagisa's question: why do we need to handle the case where protoc isn't installed? It's actually quite difficult to provide a reasonable error message when the loader is the problem, as opposed to protoc not being available, to which we could respond with protoc not found, please install or some such message.

@cpcloud
Copy link

cpcloud commented Oct 4, 2019

A compromise might be to prefer the protoc from PATH before the bundled one.

@cpcloud
Copy link

cpcloud commented Oct 7, 2019

@danburkert Would you accept a PR to move the order of preference from the bundled protoc to the one found in PATH?

@exFalso
Copy link

exFalso commented Mar 25, 2020

Just hit this, had to strace to track down the issue. I don't quite understand why prost is using a pre-compiled binary blob at all?
This choice has serious security implications. I can audit the packages installed on my system, they can all be traced back to sources. I can audit all my rust dependencies, again, the produced binaries are linked to sources. What I cannot do is audit a random binary blob, I have to trust both the upstream maintainers (and their build servers) and the prost maintainers.

(Using the PROTOC/PROTOC_INCLUDE workaround now)

@danburkert
Copy link
Collaborator

danburkert commented Mar 25, 2020

@danburkert Would you accept a PR to move the order of preference from the bundled protoc to the one found in PATH?

No, but an additional option on prost_build::Config to override the protoc used would be fine.

@danburkert
Copy link
Collaborator

I still consider it a bug that prost fails to use the bundled protoc on alpine without falling back the $PATH, but it's not something that's high on my list to fix, I will accept PRs fixing that as well though.

@xhebox
Copy link

xhebox commented Dec 24, 2020

@danburkert Hit the bug, is it acceptable to do something like this? We can run the binary to test if it works on the platform.

@@ -47,7 +48,14 @@ fn bundled_protoc() -> Option<PathBuf> {
         _ => return None,
     };
 
-    Some(bundle_path().join(protoc_bin_name))
+    let path = bundle_path().join(protoc_bin_name);
+
+    let output = Command::new(&path).arg("--version").output();
+
+    match output.map_or(false, |o| o.status.success()) {
+        true => Some(path),
+        _ => None,
+    }
 }
 
 /// Returns the path to the `protoc` included on the `PATH`, if it exists.

I would like to file the PR and test.

@danburkert
Copy link
Collaborator

@xhebox I recommend setting env PROTOC=protoc to get the behavior you want.

@danburkert
Copy link
Collaborator

This choice has serious security implications. I can audit the packages installed on my system, they can all be traced back to sources. I can audit all my rust dependencies, again, the produced binaries are linked to sources. What I cannot do is audit a random binary blob, I have to trust both the upstream maintainers (and their build servers) and the prost maintainers.

Just in case anyone stumbles across this and gets the wrong impression, the prost project is not shipping a 'random' protoc binary. The bundled binaries are taken directly from the upstream protobuf project using this script. The binaries can thus be audited against the upstream release. If there are security concerns with the upstream release, I suggest you take it there. Ultimately if this is unsatisfactory, the env escape hatch is recommended.

@xhebox
Copy link

xhebox commented Dec 24, 2020

@danburkert Yeah, I can set PROTOC somehow, github.com/AdelieLinux/gcompat can even get the executable runnable under musl-host linux. But that No such file or directory tip is just too confusing. E.g. tikv/pprof-rs is using this project, then that error poped up.

It is not working by default, and I don't even know how to fix it before digging deep. I would prefer adding some switches to move env-protoc before the bundled, or not to use the bundled one on musl(it is not working anyway).

Fallback is also a good alternative:

I still consider it a bug that prost fails to use the bundled protoc on alpine without falling back the $PATH.

The most simple way I can come up with, is running the binary directly. Then we can know if we should fallback. Maybe you can check if there is /lib{,32,64}/ld-linux-*.so.* if target_env = musl, if not, it is definitely impossible to run these executables. But as @mzabaluev mentioned:

It may be easier to try to run ldd and parse its output.

Running binary is equaivalent to ldd method, since the first thing before running is loading. And musl host does not necessarily come with a ldd program(you may not know where it is due to PATH). So running binary is the easiest and most reliable way overall.

Which way do you think is acceptable? It is just not good if prost-build is not working by default on musl.

@xhebox
Copy link

xhebox commented Dec 28, 2020

Nice!

@danburkert
Copy link
Collaborator

Yep sorry I forgot to update this thread - I figured out there's a pretty bullet-proof way to fall back to the protoc on the $PATH on musl build hosts. I also updated the error that occurs when protoc isn't invocable. Both implemented in #410 .

@nagisa
Copy link
Contributor

nagisa commented Apr 12, 2021

This continues to be a major pain on NixOS hosts, for what its worth. Even though a perfectly working protoc binary is available in the $PATH, prost just falls over trying to run its own copy.

@danburkert
Copy link
Collaborator

@nagisa I'm not familiar with NixOS, is there a cfg that can detect it reliably, similar to how #410 uses target_env = "musl"?

@danburkert
Copy link
Collaborator

For my own edification - what's different about NixOS? Does it not ship glibc?

@danburkert
Copy link
Collaborator

Also you may want to take this to a new issue, it's easy for this stuff to get lost in the noise of a closed issue.

@nagisa
Copy link
Contributor

nagisa commented Apr 12, 2021

I did file an issue in the past (https://github.com/danburkert/prost/issues/182). Just wanted to add that the fix that was made for musl systems does not cover other weird systems (i.e. is not a fix for #182 which is mentioned here)

@exFalso
Copy link

exFalso commented Apr 12, 2021

@nagisa

  PROTOC="${pkgs.protobuf}/bin/protoc";
  PROTOC_INCLUDE="${pkgs.protobuf}/include";

gets around the weird blob execution.

Want to reiterate that running a third party binary blob is a security issue, and it's also completely unnecessary.

@exFalso
Copy link

exFalso commented Apr 12, 2021

@danburkert NixOS ships glibc, but it has a completely different filesystem layout than most distributions (package paths are basically prefixed with an integrity hash of the package build code, and are completely immutable). So much so that even if the upstream maintainers made the protobuf binary statically linked, it still wouldn't work because the linux loader's location is also at such a path, and the default burnt-in location (/lib64/ld-linux-x86-64.so.2) doesn't work.

@jbg
Copy link

jbg commented Apr 24, 2021

prost is a 5th-order dependency of something I'm trying to build and I just lost several hours to this issue on an Alpine build host, with the cryptic "no such file or directory" with no context because the bundled binary has the wrong loader for Alpine. Why does prost prefer its bundled protoc even if there's one on PATH?

It's really unpleasant to have random broken binaries shipped inside crates overriding the perfectly-working ones I've already provided in the environment. To be honest, it's really unpleasant to have random binaries shipped inside crates overriding my environment even if they worked fine.

@basile-henry
Copy link

I just hit this issue on NixOS as well. Thanks for the env var fix, it works!
A better error message would be appreciated though, as it also took me looking though strace to find the culprit was prost_build and ending up in this issue. 😅

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 a pull request may close this issue.

8 participants