Hacker News Reader

Comment by jstanley | Hacker News Reader

What does the dot do?

The function is “check_c_source_compiles”. The comment indicates that the intention is to confirm that the Landlock functionality can be compiled on the system, in which case it will be enabled.

The stray dot isn’t valid C, so it will never compile. By ensuring it can never compile, Landlock will never be enabled.

Shouldn't there be a unit test to confirm landlock is on/off? (I mean, this seems a crucial aspect of the code which needs 100% test coverage.)

This is not something that a unit test can catch. First, this 100% coverage rule applies to the program/library code, and only to the subset that is not ifdeffed out (otherwise, you will not be able to have, e.g., Windows-specific code), and definitely not to the build system's code. Second, how would you test that landlock works, in a unit test, when this feature is optional and depends on the system headers being recent enough? You can't fail a unit test just because the software is being compiled on an old but still supported system, so it would be a "SKIPPED" result at best, which is not a failure and which is not normally caught.

The proper way would be to have a minimum glibc version (or whatever it depends on) where you expect landlock to be available and then shout loudly if it is not so that you can either fix the check or correct your expectations. This isn't just for malicious users, these checks can be brittle enough that a small change in the library or even compiler update can occasionally break something. Of course this is ideal and does not match common practice. I can't even claim of doing this consistently myself although I did start that practice before this mess.

configure would print that it's not enabled, so it seems like the kind of thing people would eventually notice.

Maybe, eventually, but how many people read the reams of garbage autoconf spouts out until the feature they wanted fails to materialize?

I used to and it's actually how I got a large part of my computer skills, but unfortunately I got medicated for ADHD and became normal and can't do it anymore.

But I still think reading boring logs is a great way to understand a system. Turning on all the intermediates for a compiler (llvm ghc etc) is very educational too.

The actual compiler output from the autoconf feature test is one of the things I'd probably look at fairly early on if some feature is disabled when it shouldn't be, but I maybe have a bit more experience running into problems with this than younger folks.

> if some feature is disabled when it shouldn't be

That's my point, you're going to look at autoconf's output if a feature you're expecting is missing.

But would you think to have a test or expectation for landlock in xz? How would you even check if landlock is enabled for a process?

Looks like he only introduced that "bug" in CMake. Was there migration in progress from autoconf to cmake?

Fails the build, so that Landlock support is never enabled.

It feels like this is a poor way to determine if a feature should be enabled. There are just too many reasons it could fail, and default off on compilation failure just seems like a glaring security bug. If this is a common practice, it seems like the community should rethink this choice.

I don't think it fails the build. It's part of a test, trying to compile a bit of code. If it compiles the test is true and a certain feature is enabled. If the compilation fails the the feature is disabled.

This is quite common in build systems. I had such a test produce incorrect results in the kernel build system recently. The problem is that the tests should really look carefully for an expected error message. If the compilation fails with the expected message the test result is false. If the compilation fails for some other reason the build should fail so the developer is forced to investigate.

Disclaimer: I have not studied the xz build system in detail, just what I saw from the diff plus a bit of extrapolation.

Problem is that every compiler/compiler version will have different messages for the same error. And xz is the kind of project that gets built on really really random homegrown compilers. Maybe we need an option to make the compilers output a standard JSON or whatever...

> Maybe we need an option to make the compilers output a standard JSON or whatever...

While I like the idea, I fear this would go the same route as UserAgent strings, with everybody giving carefully crafted lies. The main issue is that bugs are only known in retrospect.

As an example, suppose Compiler A and Compiler B both implement Feature X. Therefore, they both produce JSON output indicating that they support Feature X. A year later, developers for Compiler A find and fix a bug in its implementation of Feature X. Now, how do we handle that case?

* Update the version for Feature X as by Compiler A? Now Compiler B is publishing the same version as the buggy Compiler A did, and would get marked as not having a usable implementation.

* Update the spec, forcing a version update for Feature X in both Compiler A and Compiler B? Now a bugfix in one compiler requires code updates in others.

* In the build system, override the JSON output from Compiler A, such that Feature X is marked as unavailable for the buggy versions? Now the JSON is no longer trustable, as it requires maintaining a list of exceptions.

* Try to trigger the bug that Compiler A just fixed, so that the build system can recognize the buggy versions, regardless of the JSON output? Now we're back to the starting point, with features determined based on attempted compilation.

It fails that small build that is testing for landlock functionality. Hence it doesn’t build the support for it.

It doesn’t fail the overall build.

Reply on news.ycombinator.com