From 65ce95d3811cefb8c63e33e709dc0a4eda60f5d8 Mon Sep 17 00:00:00 2001 From: piegames Date: Wed, 5 Feb 2025 17:56:11 +0100 Subject: [PATCH] libexpr: Deprecate NUL bytes in strings I absolutely want to go back to allowing them, I am strongly of the opinion that nothing should be special about NUL in a string, but this will have to wait until at least the GC rewrite, so for now the least I can do is to disarm this gun. Change-Id: Id14b6037bc8b274c6c60ad970b1c74d436fb62a7 --- doc/manual/rl-next/deprecated-features.md | 1 + lix/libexpr/parser/grammar.hh | 10 +++-- lix/libexpr/parser/parser-impl1.inc.cc | 39 +++++++++++++------ lix/libexpr/parser/state.hh | 12 ++++++ lix/libutil/deprecated-features/nul-bytes.md | 6 +++ lix/libutil/meson.build | 1 + tests/functional/lang/eval-okay-nul.exp | 1 + tests/functional/lang/eval-okay-nul.flags | 1 + tests/functional/lang/eval-okay-nul.nix | Bin 0 -> 10 bytes tests/functional/lang/eval-okay-nul.out.exp | 1 + tests/functional/lang/parse-fail-nul.err.exp | 4 ++ tests/functional/lang/parse-fail-nul.nix | 1 + 12 files changed, 62 insertions(+), 15 deletions(-) create mode 100644 lix/libutil/deprecated-features/nul-bytes.md create mode 100644 tests/functional/lang/eval-okay-nul.exp create mode 100644 tests/functional/lang/eval-okay-nul.flags create mode 100644 tests/functional/lang/eval-okay-nul.nix create mode 100644 tests/functional/lang/eval-okay-nul.out.exp create mode 100644 tests/functional/lang/parse-fail-nul.err.exp create mode 120000 tests/functional/lang/parse-fail-nul.nix diff --git a/doc/manual/rl-next/deprecated-features.md b/doc/manual/rl-next/deprecated-features.md index f30b05d56..0d4e7a606 100644 --- a/doc/manual/rl-next/deprecated-features.md +++ b/doc/manual/rl-next/deprecated-features.md @@ -10,3 +10,4 @@ This release cycle features a new batch of deprecated (anti-)features. You can opt in into the old behavior with `--extra-deprecated-features` or any equivalent configuration option. - `cr-line-endings`: Current handling of CR (`\r`) or CRLF (`\r\n`) line endings in Nix is inconsistent and broken, and will lead to unexpected evaluation results with certain strings. Given that fixing the semantics might silently alter the evaluation result of derivations, the only option at the moment is to disallow them alltogether. More proper support for CRLF is planned to be added back again in the future. Until then, all files must use `\n` exclusively. +- `nul-bytes`: Currently the Nix grammar allows NUL bytes (`\0`) in strings, and thus indirectly also in identifiers. Unfortunately, several core parts of the code base still work with NUL-terminated strings and cannot easily be migrated. Also note that it is still possible to introduce NUL bytes and thus problematic behavior via other means, those are tracked separately. diff --git a/lix/libexpr/parser/grammar.hh b/lix/libexpr/parser/grammar.hh index 83b3bfc0f..d653691ca 100644 --- a/lix/libexpr/parser/grammar.hh +++ b/lix/libexpr/parser/grammar.hh @@ -249,6 +249,7 @@ struct _ind_string { /* Marker for non-empty lines */ struct has_content : semantic, seq<> {}; struct cr : semantic, one<'\r'> {}; + struct nul : semantic, one<'\0'> {}; }; struct ind_string : _ind_string, seq< TAO_PEGTL_STRING("''"), @@ -266,13 +267,14 @@ struct ind_string : _ind_string, seq< _ind_string::literal< plus< sor< - not_one<'$', '\'', '\n', '\r'>, + not_one<'$', '\'', '\n', '\r', '\0'>, // TODO probably factor this out like the others for performance - seq, not_one<'{', '\'', '\n', '\r'>>, + seq, not_one<'{', '\'', '\n', '\r', '\0'>>, seq, at>>, - seq, not_one<'\'', '$', '\n', '\r'>>, + seq, not_one<'\'', '$', '\n', '\r', '\0'>>, seq, at>>, - _ind_string::cr + _ind_string::cr, + _ind_string::nul > > >, diff --git a/lix/libexpr/parser/parser-impl1.inc.cc b/lix/libexpr/parser/parser-impl1.inc.cc index 625c417c7..4b311a6c7 100644 --- a/lix/libexpr/parser/parser-impl1.inc.cc +++ b/lix/libexpr/parser/parser-impl1.inc.cc @@ -508,7 +508,7 @@ struct StringState : SubexprState { // FIXME this truncates strings on NUL for compat with the old parser. ideally // we should use the decomposition the g gives us instead of iterating over // the entire string again. - static void unescapeStr(std::string & str) + void unescapeStr(std::string & str, State & ps) { char * s = str.data(); char * t = s; @@ -529,24 +529,26 @@ struct StringState : SubexprState { else *t = c; t++; } + if (!ps.featureSettings.isEnabled(Dep::NulBytes) && size_t(s - str.data() - 1) != str.size()) + ps.nulFound(currentPos); str.resize(t - str.data()); } - void endLiteral() + void endLiteral(State & ps) { if (!currentLiteral.empty()) { - unescapeStr(currentLiteral); + unescapeStr(currentLiteral, ps); parts.emplace_back(currentPos, std::make_unique(currentPos, std::move(currentLiteral))); } } - std::unique_ptr finish() + std::unique_ptr finish(State & ps) { if (parts.empty()) { - unescapeStr(currentLiteral); + unescapeStr(currentLiteral, ps); return std::make_unique(currentPos, std::move(currentLiteral)); } else { - endLiteral(); + endLiteral(ps); auto pos = parts[0].first; return std::make_unique(pos, true, std::move(parts)); } @@ -570,21 +572,23 @@ template<> struct BuildAST { template<> struct BuildAST { static void apply(const auto & in, StringState & s, State & ps) { - s.endLiteral(); + s.endLiteral(ps); s.parts.emplace_back(ps.at(in), s->popExprOnly()); } }; template<> struct BuildAST { static void apply(const auto & in, StringState & s, State & ps) { + if (!ps.featureSettings.isEnabled(Dep::NulBytes) && *in.begin() == '\0') + ps.nulFound(ps.at(in)); s.append(ps.at(in), "\\"); // FIXME compat with old parser s.append(ps.at(in), in.string_view()); } }; template<> struct BuildAST : change_head { - static void success0(StringState & s, ExprState & e, State &) { - e.pushExpr(noPos, s.finish()); + static void success0(StringState & s, ExprState & e, State & ps) { + e.pushExpr(noPos, s.finish(ps)); } }; @@ -619,6 +623,12 @@ template<> struct BuildAST { case 'n': s.lines.back().parts.emplace_back(ps.at(in), "\n"); break; case 'r': s.lines.back().parts.emplace_back(ps.at(in), "\r"); break; case 't': s.lines.back().parts.emplace_back(ps.at(in), "\t"); break; + case 0: + if (!ps.featureSettings.isEnabled(Dep::NulBytes)) { + ps.nulFound(ps.at(in)); + break; + } + KJ_FALLTHROUGH; default: s.lines.back().parts.emplace_back(ps.at(in), in.string_view()); break; } } @@ -637,6 +647,13 @@ template<> struct BuildAST { } }; +template<> struct BuildAST { + static void apply(const auto & in, IndStringState & s, State & ps) { + if (!ps.featureSettings.isEnabled(Dep::NulBytes)) + ps.nulFound(ps.at(in)); + } +}; + template<> struct BuildAST : change_head { static void success(const auto & in, IndStringState & s, ExprState & e, State & ps) { e.pushExpr(noPos, ps.stripIndentation(ps.at(in), std::move(s.lines))); @@ -646,7 +663,7 @@ template<> struct BuildAST : change_head struct BuildAST> { static void apply(const auto & in, StringState & s, State & ps) { s.append(ps.at(in), in.string_view()); - s.endLiteral(); + s.endLiteral(ps); } }; @@ -708,7 +725,7 @@ template<> struct BuildAST : change_head { } static void success(const auto & in, StringState & s, ExprState & e, State & ps) { - s.endLiteral(); + s.endLiteral(ps); check_slash(ps.atEnd(in), s, ps); check_slash(ps.atEnd(in), s, ps); if (s.parts.size() == 1) { diff --git a/lix/libexpr/parser/state.hh b/lix/libexpr/parser/state.hh index e2c7754fe..6153fcddf 100644 --- a/lix/libexpr/parser/state.hh +++ b/lix/libexpr/parser/state.hh @@ -37,6 +37,7 @@ struct State void dupAttr(Symbol attr, const PosIdx pos, const PosIdx prevPos); void overridesFound(const PosIdx pos); void badLineEndingFound(const PosIdx pos, bool warnOnly); + void nulFound(const PosIdx pos); void addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_ptr e, const PosIdx pos); void validateLambdaAttrs(AttrsPattern & pattern, PosIdx pos = noPos); std::unique_ptr stripIndentation(const PosIdx pos, std::vector && line); @@ -114,6 +115,17 @@ inline void State::badLineEndingFound(const PosIdx pos, bool warnOnly) .pos = positions[pos], }); } +// Added 2025-02-05. +inline void State::nulFound(const PosIdx pos) +{ + throw ParseError({ + .msg = HintFmt( + "NUL bytes (`\\0`) are currently not well supported, because internally strings are NUL-terminated, which may lead to unexpected truncation. Use %s to disable this error.", + "--extra-deprecated-features nul-bytes" + ), + .pos = positions[pos], + }); +} inline void State::addAttr(ExprAttrs * attrs, AttrPath && attrPath, std::unique_ptr e, const PosIdx pos) { diff --git a/lix/libutil/deprecated-features/nul-bytes.md b/lix/libutil/deprecated-features/nul-bytes.md new file mode 100644 index 000000000..483f88042 --- /dev/null +++ b/lix/libutil/deprecated-features/nul-bytes.md @@ -0,0 +1,6 @@ +--- +name: nul-bytes +internalName: NulBytes +--- +Allow NUL bytes (`\0`) in Nix strings. +Note however that due to Nix using NUL-terminated strings internally, this may cause undefined behavior. diff --git a/lix/libutil/meson.build b/lix/libutil/meson.build index 507f0f548..6fcd8cb76 100644 --- a/lix/libutil/meson.build +++ b/lix/libutil/meson.build @@ -156,6 +156,7 @@ experimental_feature_definitions = files( deprecated_feature_definitions = files( 'deprecated-features/ancient-let.md', 'deprecated-features/cr-line-endings.md', + 'deprecated-features/nul-bytes.md', 'deprecated-features/rec-set-overrides.md', 'deprecated-features/url-literals.md', 'deprecated-features/shadow-internal-symbols.md', diff --git a/tests/functional/lang/eval-okay-nul.exp b/tests/functional/lang/eval-okay-nul.exp new file mode 100644 index 000000000..810c96eee --- /dev/null +++ b/tests/functional/lang/eval-okay-nul.exp @@ -0,0 +1 @@ +"foo" diff --git a/tests/functional/lang/eval-okay-nul.flags b/tests/functional/lang/eval-okay-nul.flags new file mode 100644 index 000000000..33da0f779 --- /dev/null +++ b/tests/functional/lang/eval-okay-nul.flags @@ -0,0 +1 @@ +--extra-deprecated-features nul-bytes diff --git a/tests/functional/lang/eval-okay-nul.nix b/tests/functional/lang/eval-okay-nul.nix new file mode 100644 index 0000000000000000000000000000000000000000..3689409171139a7d310eac52df0be4315b6ba786 GIT binary patch literal 10 RcmY#N%g<*>N-R?10ss()0>}UW literal 0 HcmV?d00001 diff --git a/tests/functional/lang/eval-okay-nul.out.exp b/tests/functional/lang/eval-okay-nul.out.exp new file mode 100644 index 000000000..257cc5642 --- /dev/null +++ b/tests/functional/lang/eval-okay-nul.out.exp @@ -0,0 +1 @@ +foo diff --git a/tests/functional/lang/parse-fail-nul.err.exp b/tests/functional/lang/parse-fail-nul.err.exp new file mode 100644 index 000000000..a04ce99a8 --- /dev/null +++ b/tests/functional/lang/parse-fail-nul.err.exp @@ -0,0 +1,4 @@ +error: NUL bytes (`\0`) are currently not well supported, because internally strings are NUL-terminated, which may lead to unexpected truncation. Use --extra-deprecated-features nul-bytes to disable this error. + at «stdin»:1:2: + 1| "foo + | ^ diff --git a/tests/functional/lang/parse-fail-nul.nix b/tests/functional/lang/parse-fail-nul.nix new file mode 120000 index 000000000..9b0527d33 --- /dev/null +++ b/tests/functional/lang/parse-fail-nul.nix @@ -0,0 +1 @@ +eval-okay-nul.nix \ No newline at end of file