fix(libmain/common-args): do not exceed maximum allowed verbosity

[xokdvium]

Motivation

This patch gets rid of UB when verbosity exceeds the maximum logging value of lvlVomit = 7 and
reaches invalid values (e.g. 8). This is actually triggered in functional tests.
There are too many occurrences to list, but here's one from the UBSAN log:

../src/libstore/gc.cc:610:5: runtime error: load of value 8, which is not a valid value for type 'Verbosity'

Context

#10969

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[edolstra]

This is extremely verbose, so I'd really prefer to stick with an enum. Is there any real-world scenario where it being an enum causes a problem?

If we want to keep UBSAN happy, we could use a type specifier for the enum (enum Verbosity : uint32_t for instance).

[xokdvium]

This is extremely verbose, so I'd really prefer to stick with an enum. Is there any real-world scenario where it being an enum causes a problem?

If we want to keep UBSAN happy, we could use a type specifier for the enum (enum Verbosity : uint32_t for instance).

I'm not sure I'm following you about keeping UBSAN happy. Having an enum with an incorrect value is UB and is absolutely an error. Making the underlying type a uint32_t wouldn't change anything. The problem is in the verbosity + 1, which does not clamp the value in range.

I debated just fixing the bug and clamp the value in the -v handler, but I wanted to make this more foolproof. Using an enum is okay, with since it's a plain C enum it's implicitly convertible from an int and it's very easy to assign an incorrect value to it. Using an enum class would get rid of unsafe conversions, but would introduce usability issues.

I do agree that this is a bit verbose, but has the benefit of guarding against unsafe implicit conversions from integral types and giving safe member functions and constructors. Now it's impossible to accidentally construct/assign an invalid Verbosity.

As an alternative we could change it to be an enum class with safe freestanding functions. I'm not sure it would be better in terms of readability. If that's also too big of a burden then I'll just revert the change and fix the underlying bug.

IMHO this approach is best for conciseness and barely affects user code, but that's just my personal opinion.

[edolstra]

Having an enum with an incorrect value is UB and is absolutely an error.

No. This is simply not a real-world issue. What's the problem here? That a user might specify -v 4 billion times and overflow the enum?

[xokdvium]

Sorry, I might have phrased my thoughts incorrectly. By overflow I meant no the integer overflow but the fact that the enum value is illegal. E.g. by specifying the -v option 8 times the underlying value becomes too large to be represented by enum and does not saturate to the maximum allowed value.

Here the verbosity does not get clamped to the maximum allowed value lvlVomit but instead exceeds 7 and is cast to Verbosity (which is UB, since it does not correspond to any enumeration value).

This actually gets asserted in some places:

nix/src/libutil/error.cc

Lines 266 to 267 in 14edb78

	default:
	assert(false);

Here's the minified fix to clarify what the purpose is:

diff --git a/src/libmain/common-args.cc b/src/libmain/common-args.cc
index 768b2177c..9306acac4 100644
--- a/src/libmain/common-args.cc
+++ b/src/libmain/common-args.cc
@@ -17,7 +17,7 @@ MixCommonArgs::MixCommonArgs(const std::string & programName)
         .shortName = 'v',
         .description = "Increase the logging verbosity level.",
         .category = loggingCategory,
-        .handler = {[]() { verbosity = (Verbosity) (verbosity + 1); }},
+        .handler = {[]() { verbosity = (Verbosity) std::min<std::underlying_type_t<Verbosity>>(verbosity + 1, lvlVomit); }},
     });
 
     addFlag({

Sorry for the poor phrasing.

[Mic92]

Here the verbosity does not get clamped to the maximum allowed value lvlVomit but instead exceeds 7 and is cast to Verbosity (which is UB, since it does not correspond to any enumeration value).

This actually gets asserted in some places:

nix/src/libutil/error.cc

Lines 266 to 267 in 14edb78

	default:
	assert(false);

Here's the minified fix to clarify what the purpose is:

diff --git a/src/libmain/common-args.cc b/src/libmain/common-args.cc
index 768b2177c..9306acac4 100644
--- a/src/libmain/common-args.cc
+++ b/src/libmain/common-args.cc
@@ -17,7 +17,7 @@ MixCommonArgs::MixCommonArgs(const std::string & programName)
         .shortName = 'v',
         .description = "Increase the logging verbosity level.",
         .category = loggingCategory,
-        .handler = {[]() { verbosity = (Verbosity) (verbosity + 1); }},
+        .handler = {[]() { verbosity = (Verbosity) std::min<std::underlying_type_t<Verbosity>>(verbosity + 1, lvlVomit); }},
     });
 
     addFlag({

Sorry for the poor phrasing.

The minimal fix sound good to me. @edolstra any concerns about that?

[Mic92]

In the nix meeting @edolstra said, he would be fine with the one-line fix as well.

[xokdvium]

@Mic92, pushed the minimal fix. Sorry once again for poor wording and for going a bit over the top with the initial patch.

[Mic92]

@mergify queue

[mergify]

queue

🟠 Waiting for conditions to match

• -closed [📌 queue requirement]
• -conflict [📌 queue requirement]
• -draft [📌 queue requirement]
• any of: [📌 queue -> configuration change requirements]
  • -mergify-configuration-changed
  • check-success = Configuration changed
• any of: [🔀 queue conditions]
  • all of: [📌 queue conditions of queue default]
    • any of: [🛡 GitHub branch protection]
      • check-skipped = installer_test
      • check-neutral = installer_test
      • check-success = installer_test
    • any of: [🛡 GitHub branch protection]
      • check-success = tests (ubuntu-latest)
      • check-neutral = tests (ubuntu-latest)
      • check-skipped = tests (ubuntu-latest)
    • any of: [🛡 GitHub branch protection]
      • check-success = tests (macos-latest)
      • check-neutral = tests (macos-latest)
      • check-skipped = tests (macos-latest)

has been addressed.