linux_android_with_fallback: detect getrandom #758
Conversation
Running CI on any branch is more ergonomic, particularly for folks using CI in their forks.
tamird
force-pushed
the
dlsym-fallback-static
branch
from
de32ca5 to
88baf82
Compare
Avoid dlsym when statically linking crt.
tamird
force-pushed
the
dlsym-fallback-static
branch
from
88baf82 to
edca8cf
Compare
There was a problem hiding this comment.
I don't think I like the proposed approach. Not only it's significantly more complex, but I am also not sure about its universality. IIUC it checks that getrandom is available on local machine, but compiled binary may be executed in a completely different environment!
Until the minimum supported Linux kernel version is bumped by Rust, I don't think we should statically link to the getrandom symbol.
|
|
||
| on: | ||
| push: | ||
| branches: master |
There was a problem hiding this comment.
These changes are not relevant.
There was a problem hiding this comment.
Sure, they're in a separate commit with reasoning given (it's a bummer you guys insist on squashing on merge).
There was a problem hiding this comment.
@tamird I think this is a reasonable thing to do, but changing on which branches CI runs is tricky from a security perspective, and it deserves a dedicated PR.
|
Also, it's better to move the NetBSD cleanup into a separate PR. |
|
And don't forget that we have |
Hm. I don't think I follow you here - we're checking if getrandom is available statically on the target, i.e. if we're statically linking against MUSL then getrandom is available statically. But we still check at runtime that the target kernel allows it. |
|
In my understanding, you check that you can link statically on your host, so your build script will happily link to glibc's getrandom (to which your application links implicitly when you compile for GNU Linux targets) and will erroneously report that static linking can be used just fine, while the resulting application could be then deployed on a system with old glibc and Linux kernel. Even if I misunderstand something and it works correctly in all corner cases, I don't think the additional complexity is justified by the arguably very minor benefit. |
| static GETRANDOM_FN: lazy::LazyUsize = lazy::LazyUsize::new(); | ||
|
|
||
| let fptr = GETRANDOM_FN.unsync_init(init); | ||
| let fptr = unsafe { mem::transmute::<usize, GetRandomFn>(fptr) }; |
There was a problem hiding this comment.
Note that this is probably incorrect from the strict pointer provenance point of view.
There was a problem hiding this comment.
I don't think so. The provenance of the original pointer is the same as the provenance of the pointer constructed here.
I don't understand this. If static linking is used, what does it matter what libc is on the target that is deployed to? Yes, the kernel might not support getrandom, but that is why we still check it at runtime using
Well, that's insurmountable. If you are not open to persuasion, then there's nothing to discuss. |
Moved to #759. |
|
|
||
| on: | ||
| push: | ||
| branches: master |
There was a problem hiding this comment.
@tamird I think this is a reasonable thing to do, but changing on which branches CI runs is tricky from a security perspective, and it deserves a dedicated PR.
|
I missed the So unless @josephlr has a different opinion, I think we should not use this approach. |
3 participants
Avoid dlsym when statically linking crt.
Relative to #598 this PR keeps dlsym in place when linking dynamically and avoids the dependency on a C compiler. The benefits are as before: avoiding an indirect call when linking statically.
This also contains minor cleanup in
src/backends/netbsd.rs.