[liblzma] Add sandbox feature#50072
Conversation
2cc75b1 to
5b70779
Compare
9d92d8e to
2f0cbec
Compare
2f0cbec to
5fbc89f
Compare
BillyONeal
left a comment
There was a problem hiding this comment.
To get in this condition you have to be writing a triplet to pass in the -fsanitize, and the bug also describes a way to do this without introducing a feature.in the triplet. I'm not sure adding this feature actually fixes a problem?
| } | ||
| ], | ||
| "default-features": [ | ||
| "sandbox" |
There was a problem hiding this comment.
In general we have taken a dim view of the default-features feature because user feedback has been that they feel disabling default features is impossible; this inserts a feature constraint into almost every existing use of "liblzma" in the registry. I think this needs to be a non-default feature unless the library is entirely useless without it.
There was a problem hiding this comment.
I think this needs to be a non-default feature unless the library is entirely useless without it.
But what is the desired default configuration: sandbox on or sandbox off? Is it related to security?
IIRC the "dim view" on default-features is also related to that user would need to know what they can turn off or which ports must be fixed when maintainers make poor choices. IMO the absence of default features means that users must know or study all dependencies to ensure upstreams' default security and functional capabilities are actually engaged.
There was a problem hiding this comment.
I agree with you both, that why we need a way that user can choose not install a specific feature, even if it a default.
There was a problem hiding this comment.
something like: liblzma[!sandbox]
There was a problem hiding this comment.
I set sandbox to default because it's enabled by default.
I dug a little. Sandbox is used only by tools. Since vcpkg is mainly used to build lib and not tools / executables, the sandbox feature can be disabled by default.
I will open an issue in the xz project to not generate a fatal error if sanitization is enabled with XZ_SANDBOX set to "auto" to just disable the sandbox.
I don't like adding a port specification in triplet. That's why I implemented it as a feature. But at second thought, I could also fixed it by patching the source to not fail if XZ_SANDBOX is set to auto and to disable sandbox feature. What do you prefer ? |
|
I reported the problem to upstream and sent a MR |
|
Patching CMakeLists.txt is not a good solution. So there are two solutions:
Do you have a preference? |
JavierMatosD
left a comment
There was a problem hiding this comment.
Sandbox is used only by tools.
If Sandbox only affects tools feature, then should it be a part of the tools feature logic internally rather than exposing it as a user facing feature? Something like "if tools is enabled, sandbox is auto; if not, no". I think that could work if you can show proof that this only affects tools. I'd still be concerned with anyone who wants tools but doesn't want Sandbox would be unable to disable it easily. That said, upstream enables it by default. I agree with @BillyONeal that this should not be a default feature.
It seems upstream intentionally kept this an error -> tukaani-project/xz#209 (comment) "A fair bit of thinking and discussion occurred before tukaani-project/xz@88588b1 was committed in late 2023. I think the commit is still good, and "auto" should behave like it does now."
|
In
|
5fbc89f to
e4a8f3e
Compare
e4a8f3e to
af763f0
Compare
Fixes #49986
./vcpkg x-add-version --alland committing the result.