diff --git a/doc/manual/rl-next/pascal-strings.md b/doc/manual/rl-next/pascal-strings.md new file mode 100644 index 000000000..d1e37ad85 --- /dev/null +++ b/doc/manual/rl-next/pascal-strings.md @@ -0,0 +1,47 @@ +--- +synopsis: "Strings may now contain NUL bytes" +cls: [3968] +issues: [] +category: "Breaking Changes" +credits: [horrors] +--- + +Lix now allows strings to contain NUL bytes instead of silently truncating the +string before the first such byte. Notably NUL-bearing strings were allowed as +attribute names—even though the corresponding strings were not representable!— +leading to very surprising and incorrect behavior in corner cases, for example + +``` +nix-repl> builtins.fromJSON ''{"a": 1, "a\u0000b": 2}'' +{ + a = 1; + "ab" = 2; +} + +nix-repl> builtins.attrNames (builtins.fromJSON ''{"a": 1, "a\u0000b": 2}'') +[ + "a" + "a" +] +``` + +rather than the more correct but still with the terminal eating NUL on display + +``` +nix-repl> builtins.fromJSON ''{"a": 1, "a\u0000b": 2}'' +{ + a = 1; + "ab" = 2; +} + +nix-repl> builtins.attrNames (builtins.fromJSON ''{"a": 1, "a\u0000b": 2}'') +[ + "a" + "ab" +] +``` + +We consider this a breaking change since eval results *will* change if strings +with embedded NUL bytes were used, but we also consider the old behavior to be +not intentional (seeing how inconsistent it was) but merely fallout from a old +and misguided implementation decision to be worked around, not actually fixed. diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index ed02a1e01..e26c4bf8e 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -2014,17 +2014,16 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) for (const auto & part : s) result += *part; return result; }; - /* c_str() is not str().c_str() because we want to create a string - Value. allocating a GC'd string directly and moving it into a - Value lets us avoid an allocation and copy. */ - const auto c_str = [&] { - char * result = gcAllocString(sSize + 1); - char * tmp = result; + /* build a gc'd value string directly instead of going through str() + and mkString to save an allocation and copy */ + const auto gcStr = [&] { + auto result = Value::Str::gcAlloc(sSize); + + char * tmp = result->contents; for (const auto & part : s) { memcpy(tmp, part->data(), part->size()); tmp += part->size(); } - *tmp = 0; return result; }; @@ -2098,7 +2097,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) state.ctx.errors.make("a string that refers to a store path cannot be appended to a path").atPos(pos).withFrame(env, *this).debugThrow(); v.mkPath(CanonPath(canonPath(str()))); } else - v.mkStringMove(c_str(), context); + v.mkStringMove(gcStr(), context); } @@ -2369,8 +2368,8 @@ BackedStringView EvalState::coerceToString( if (v.type() == nPath) { return !canonicalizePath && !copyToStore - ? v.string().content // FIXME: hack to preserve path literals that end in a slash, as in - // /foo/${x}. + // FIXME: hack to preserve path literals that end in a slash, as in /foo/${x}. + ? std::string(v.string().content->str()) : (copyToStore ? ctx.store->printStorePath( aio.blockOn(ctx.paths.copyPathToStore(context, v.path(), ctx.repair)) @@ -2603,8 +2602,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v return v1.str() == v2.str(); case nPath: - // NOLINTNEXTLINE(lix-unsafe-c-calls) - return strcmp(v1.string().content, v2.string().content) == 0; + return v1.string().content->str() == v2.string().content->str(); case nNull: return true; diff --git a/lix/libexpr/nixexpr.hh b/lix/libexpr/nixexpr.hh index 1fc708a5e..8cfccfeaf 100644 --- a/lix/libexpr/nixexpr.hh +++ b/lix/libexpr/nixexpr.hh @@ -204,21 +204,31 @@ struct ExprFloat : ExprLiteral struct ExprString : ExprLiteral { - std::string s; - Value::String strcb{.content = s.c_str(), .context = nullptr}; - ExprString(const PosIdx pos, std::string && s) : ExprLiteral(pos), s(std::move(s)) + std::unique_ptr contents; + Value::String strcb{.content = contents.get(), .context = nullptr}; + ExprString(const PosIdx pos, std::string s) : ExprLiteral(pos), contents(Value::Str::copy(s)) { v = {NewValueAs::string, &strcb}; } + + std::string_view str() const + { + return contents->str(); + } }; struct ExprPath : ExprLiteral { - std::string s; - Value::String strcb{.content = s.c_str(), .context = Value::String::path}; - ExprPath(const PosIdx pos, std::string s) : ExprLiteral(pos), s(std::move(s)) + std::unique_ptr contents; + Value::String strcb{.content = contents.get(), .context = Value::String::path}; + ExprPath(const PosIdx pos, std::string s) : ExprLiteral(pos), contents(Value::Str::copy(s)) { - v = {NewValueAs::path, &strcb}; + v = Value{NewValueAs::path, &strcb}; + } + + std::string_view str() const + { + return contents->str(); } }; diff --git a/lix/libexpr/parser/parser-impl1.inc.cc b/lix/libexpr/parser/parser-impl1.inc.cc index 113471ab9..b81c23477 100644 --- a/lix/libexpr/parser/parser-impl1.inc.cc +++ b/lix/libexpr/parser/parser-impl1.inc.cc @@ -326,10 +326,11 @@ template<> struct BuildAST { template<> struct BuildAST { static void apply(const auto & in, auto & s, State & ps) { auto e = s->popExprOnly(); - if (auto str = dynamic_cast(e.get())) - s.pushAttr(ps.symbols.create(str->s), ps.at(in)); - else + if (auto estr = dynamic_cast(e.get())) { + s.pushAttr(ps.symbols.create(estr->str()), ps.at(in)); + } else { s.pushAttr(std::move(e), ps.at(in)); + } } }; @@ -387,9 +388,9 @@ template<> struct BuildAST : change_head { for (auto & i : s.attrs) { if (i.symbol) continue; - if (auto str = dynamic_cast(i.expr.get())) - i = AttrName(i.pos, ps.symbols.create(str->s)); - else { + if (auto estr = dynamic_cast(i.expr.get())) { + i = AttrName(i.pos, ps.symbols.create(estr->str())); + } else { throw ParseError({ .msg = HintFmt("dynamic attributes not allowed in inherit"), .pos = ps.positions[i.pos] @@ -770,13 +771,15 @@ template<> struct BuildAST : change_head { template static void check_slash(PosIdx end, StringState & s, State & ps) { auto e = dynamic_cast(s.parts.back().second.get()); - if (!e || !e->s.ends_with('/')) + if (!e || !e->str().ends_with('/')) { return; - if (s.parts.size() > 1 || e->s != "/") + } + if (s.parts.size() > 1 || e->str() != "/") { throw ParseError({ .msg = HintFmt("path has a trailing slash"), .pos = ps.positions[end], }); + } } static void success(const auto & in, StringState & s, ExprState & e, State & ps) { diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index 53eaecaf6..01007c0db 100644 --- a/lix/libexpr/primops.cc +++ b/lix/libexpr/primops.cc @@ -485,8 +485,7 @@ struct CompareValues : NeverAsync case nString: return v1.str() < v2.str(); case nPath: - // NOLINTNEXTLINE(lix-unsafe-c-calls) - return strcmp(v1.string().content, v2.string().content) < 0; + return v1.string().content->str() < v2.string().content->str(); case nList: // Lexicographic comparison for (size_t i = 0;; i++) { @@ -1356,11 +1355,6 @@ static void prim_readFile(EvalState & state, Value * * args, Value & v) { auto path = realisePath(state, *args[0]); auto s = path.readFile(); - if (s.find((char) 0) != std::string::npos) - state.ctx.errors.make( - "the contents of the file '%1%' cannot be represented as a Nix string", - path - ).debugThrow(); StorePathSet refs; if (state.ctx.store->isInStore(path.canonical().abs())) { try { diff --git a/lix/libexpr/symbol-table.hh b/lix/libexpr/symbol-table.hh index 60645194e..a38fb59d2 100644 --- a/lix/libexpr/symbol-table.hh +++ b/lix/libexpr/symbol-table.hh @@ -45,7 +45,7 @@ private: /* * The type that actually stores the string contained inside of the Value. */ - std::string contents; + std::unique_ptr contents; Value::String strcb; @@ -56,8 +56,8 @@ private: public: explicit InternedSymbol(std::string_view s) - : contents(s) - , strcb{.content = contents.c_str(), .context = nullptr} + : contents(Value::Str::copy(s)) + , strcb{.content = contents.get(), .context = nullptr} , underlyingValue(NewValueAs::string, &strcb) { } @@ -69,17 +69,17 @@ public: operator SymbolStr() const { - return SymbolStr(contents); + return SymbolStr(contents->str()); } bool operator==(std::string_view s2) const { - return contents == s2; + return contents->str() == s2; } operator std::string_view() const { - return contents; + return contents->str(); } Value toValue() const diff --git a/lix/libexpr/value.cc b/lix/libexpr/value.cc index 10d38863a..2ff0bf0a6 100644 --- a/lix/libexpr/value.cc +++ b/lix/libexpr/value.cc @@ -59,9 +59,11 @@ void Value::mkPrimOp(PrimOp * p) *this = {NewValueAs::primop, *p}; } -void Value::mkString(std::string_view s) +void Value::mkString(std::string_view s, const char ** context) { - mkString(gcCopyStringIfNeeded(s)); + auto block = gcAllocType(); + *block = {.content = Str::gcCopy(s), .context = context}; + raw = tag(tString, block); } void Value::mkString(std::string_view s, const NixStringContext & context) @@ -70,10 +72,12 @@ void Value::mkString(std::string_view s, const NixStringContext & context) copyContextToValue(*untag(), context); } -void Value::mkStringMove(const char * s, const NixStringContext & context) +void Value::mkStringMove(Str * s, const NixStringContext & context) { - mkString(s); - copyContextToValue(*untag(), context); + auto block = gcAllocType(); + *block = {.content = s, .context = nullptr}; + raw = tag(tString, block); + copyContextToValue(*block, context); } void Value::mkPath(const SourcePath & path) diff --git a/lix/libexpr/value.hh b/lix/libexpr/value.hh index 0970d6fab..1f3d80f8f 100644 --- a/lix/libexpr/value.hh +++ b/lix/libexpr/value.hh @@ -7,8 +7,10 @@ #include #include #include +#include #include #include +#include #include #include "lix/libexpr/gc-alloc.hh" @@ -289,6 +291,67 @@ private: public: + /** + * Underlying data storage for stringly values (i.e., strings and paths). Stores + * both the length of the string and its contents in a single GC-allocated block + * of memory to reduce overhead in the most common case. This and `String` could + * be merged into a single struct to decrease memory overhead further, but doing + * so precludes us from using atomic allocations that do not need to be scanned, + * increasing GC runtime overhead. We only use this struct to replace C strings. + */ + struct Str + { + struct Deleter + { + void operator()(Str * s) + { + free(s); + } + }; + + size_t length; + char contents[0]; + + std::string_view str() const + { + return {contents, length}; + } + + static Str * gcAlloc(size_t size) + { + auto result = static_cast(LIX_GC_MALLOC_ATOMIC(sizeof(Value::Str) + size)); + if (result) { + result->length = size; + return result; + } + throw std::bad_alloc(); + } + + static std::unique_ptr copy(std::string_view s) + { + auto result = alloc(s.size()); + memcpy(result->contents, s.data(), s.size()); + return {result, {}}; + } + + static Str * gcCopy(std::string_view s) + { + auto result = gcAlloc(s.size()); + memcpy(result->contents, s.data(), s.size()); + return result; + } + + private: + static Str * alloc(size_t size) + { + if (auto result = static_cast(malloc(sizeof(Value::Str) + size))) { + result->length = size; + return result; + } + throw std::bad_alloc(); + } + }; + /** * Empty list constant. */ @@ -377,7 +440,7 @@ public: /// Neither the C-string nor the context array are copied; this constructor /// assumes suitable memory has already been allocated (with the GC if /// enabled), and string and context data copied into that memory. - Value(string_t, char const * strPtr, char const ** contextPtr = nullptr) + Value(string_t, const Str * strPtr, char const ** contextPtr = nullptr) { auto block = gcAllocType(); *block = {.content = strPtr, .context = contextPtr}; @@ -394,7 +457,7 @@ public: Value(string_t, std::string_view copyFrom, NixStringContext const & context = {}) { auto block = gcAllocType(); - *block = {.content = gcCopyStringIfNeeded(copyFrom), .context = nullptr}; + *block = {.content = Str::gcCopy(copyFrom), .context = nullptr}; raw = tag(tString, block); if (context.empty()) { @@ -415,39 +478,6 @@ public: block->context[n] = nullptr; } - /// Constructx a nix language value of type "string", with the value of the - /// C-string pointed to by @ref strPtr, and optionally with a set of string - /// context @ref context. - /// - /// The C-string is not copied; this constructor assumes suitable memory - /// has already been allocated (with the GC if enabled), and string data - /// has been copied into that memory. The context data *is* copied from - /// @ref context, and this constructor performs a dynamic (GC) allocation - /// to do so. - Value(string_t, char const * strPtr, NixStringContext const & context) - { - auto block = gcAllocType(); - *block = {.content = strPtr, .context = nullptr}; - raw = tag(tString, block); - - if (context.empty()) { - // It stays nullptr - return; - } - - // Copy the context. - block->context = gcAllocType(context.size() + 1); - - size_t n = 0; - for (NixStringContextElem const & contextElem : context) { - block->context[n] = gcCopyStringIfNeeded(contextElem.to_string()); - n += 1; - } - - // Terminator sentinel. - block->context[n] = nullptr; - } - /// Constructs a nix language value of type "path", with the value of the /// C-string pointed to by @ref strPtr. /// @@ -467,7 +497,7 @@ public: Value(path_t, SourcePath const & path) { auto block = gcAllocType(); - *block = {.content = gcCopyStringIfNeeded(path.canonical().abs()), .context = String::path}; + *block = {.content = Str::gcCopy(path.canonical().abs()), .context = String::path}; raw = tag(tString, block); } @@ -620,7 +650,7 @@ public: /// marker location for paths, to be used as path context. static inline const char * path[] = {"\1", nullptr}; - const char * content; + const Str * content; const char ** context; // must be in sorted order bool isPath() const @@ -733,25 +763,18 @@ public: raw = tag(tBool, b); } - inline void mkString(const char * s, const char * * context = 0) - { - auto block = gcAllocType(); - *block = {.content = s, .context = context}; - raw = tag(tString, block); - } - - void mkString(std::string_view s); + void mkString(std::string_view s, const char ** context = 0); void mkString(std::string_view s, const NixStringContext & context); - void mkStringMove(const char * s, const NixStringContext & context); + void mkStringMove(Str * s, const NixStringContext & context); void mkPath(const SourcePath & path); inline void mkPath(const char * path) { auto block = gcAllocType(); - *block = {.content = path, .context = String::path}; + *block = {.content = Str::gcCopy(path), .context = String::path}; raw = tag(tString, block); } @@ -812,13 +835,13 @@ public: SourcePath path() const { assert(internalType() == tString && untag()->isPath()); - return SourcePath{CanonPath(untag()->content)}; + return SourcePath{CanonPath(untag()->content->str())}; } std::string_view str() const { assert(internalType() == tString && !untag()->isPath()); - return std::string_view(untag()->content); + return std::string_view(untag()->content->str()); } NixInt integer() const diff --git a/lix/libutil/escape-string.cc b/lix/libutil/escape-string.cc index c550c960f..2dc70eb27 100644 --- a/lix/libutil/escape-string.cc +++ b/lix/libutil/escape-string.cc @@ -35,7 +35,7 @@ escapeString(std::ostream & output, std::string_view string, EscapeStringOptions output << "\\r"; } else if (*i == '\t') { output << "\\t"; - } else if (*i == '$' && *(i + 1) == '{') { + } else if (*i == '$' && i + 1 != string.end() && *(i + 1) == '{') { output << "\\" << *i; } else if (options.escapeNonPrinting && !isprint(*i)) { output << MaybeHexEscapedChar{*i}; diff --git a/tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromJSON-value.out.exp b/tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromJSON-value.out.exp index 231f150c5..aff126ec5 100644 Binary files a/tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromJSON-value.out.exp and b/tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromJSON-value.out.exp differ diff --git a/tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromTOML-value.out.exp b/tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromTOML-value.out.exp index 01cdaa278..843c351ca 100644 Binary files a/tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromTOML-value.out.exp and b/tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromTOML-value.out.exp differ diff --git a/tests/functional2/lang/nul_bytes_parsers/eval-depr-fromJSON-value.out.exp b/tests/functional2/lang/nul_bytes_parsers/eval-depr-fromJSON-value.out.exp index 231f150c5..aff126ec5 100644 Binary files a/tests/functional2/lang/nul_bytes_parsers/eval-depr-fromJSON-value.out.exp and b/tests/functional2/lang/nul_bytes_parsers/eval-depr-fromJSON-value.out.exp differ diff --git a/tests/functional2/lang/nul_bytes_parsers/eval-depr-fromTOML-value.out.exp b/tests/functional2/lang/nul_bytes_parsers/eval-depr-fromTOML-value.out.exp index 01cdaa278..843c351ca 100644 Binary files a/tests/functional2/lang/nul_bytes_parsers/eval-depr-fromTOML-value.out.exp and b/tests/functional2/lang/nul_bytes_parsers/eval-depr-fromTOML-value.out.exp differ diff --git a/tests/unit/libexpr-support/tests/libexpr.hh b/tests/unit/libexpr-support/tests/libexpr.hh index 17871f526..4a00a7d39 100644 --- a/tests/unit/libexpr-support/tests/libexpr.hh +++ b/tests/unit/libexpr-support/tests/libexpr.hh @@ -112,7 +112,7 @@ namespace nix { if (arg.type() != nPath) { *result_listener << "Expected a path got " << arg.type(); return false; - } else if (std::string_view(arg.string().content) != p) { + } else if (arg.string().content->str() != p) { *result_listener << "Expected a path that equals \"" << p << "\" but got: " << arg.path(); return false;