guiwf/lix
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

libexpr: use pascal strings for eval

Browse Source 
this has no performance impact in any benchmarks we've run. nul bytes
are still used as implicit truncation points in many places all over:
rejecting them in all locations that treat them as a string end point
requires large changes such as using a proper path library everywhere

Change-Id: I936158bd435f6abf009a689adfbc24496262c578
This commit is contained in:
eldritch horrors
2025-10-11 12:57:57 +02:00
parent 53d172a308
commit b88a6e6f11
14 changed files with 175 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files
doc/manual/rl-next/pascal-strings.md
+47
View File
@@ -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.
lix/libexpr/eval.cc
+10 -12
View File
@@ -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<EvalError>("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;
lix/libexpr/nixexpr.hh
+17 -7
View File
@@ -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<Value::Str, Value::Str::Deleter> 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<Value::Str, Value::Str::Deleter> 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();
}
};
lix/libexpr/parser/parser-impl1.inc.cc
+11 -8
View File
@@ -326,10 +326,11 @@ template<> struct BuildAST<grammar::v1::attr::simple> {
template<> struct BuildAST<grammar::v1::attr::string> {
static void apply(const auto & in, auto & s, State & ps) {
auto e = s->popExprOnly();
if (auto str = dynamic_cast<ExprString *>(e.get()))
s.pushAttr(ps.symbols.create(str->s), ps.at(in));
else
if (auto estr = dynamic_cast<ExprString *>(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<grammar::v1::inherit> : change_head<InheritState> {
for (auto & i : s.attrs) {
if (i.symbol)
continue;
if (auto str = dynamic_cast<ExprString *>(i.expr.get()))
i = AttrName(i.pos, ps.symbols.create(str->s));
else {
if (auto estr = dynamic_cast<ExprString *>(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<grammar::v1::path> : change_head<StringState> {
template<typename E>
static void check_slash(PosIdx end, StringState & s, State & ps) {
auto e = dynamic_cast<E *>(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) {
lix/libexpr/primops.cc
+1 -7
View File
@@ -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<EvalError>(
"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 {
lix/libexpr/symbol-table.hh
+6 -6
View File
@@ -45,7 +45,7 @@ private:
/*
* The type that actually stores the string contained inside of the Value.
*/
std::string contents;
std::unique_ptr<Value::Str, Value::Str::Deleter> 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
lix/libexpr/value.cc
+9 -5
View File
@@ -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<String>();
*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<String *>(), context);
}
void Value::mkStringMove(const char * s, const NixStringContext & context)
void Value::mkStringMove(Str * s, const NixStringContext & context)
{
mkString(s);
copyContextToValue(*untag<String *>(), context);
auto block = gcAllocType<String>();
*block = {.content = s, .context = nullptr};
raw = tag(tString, block);
copyContextToValue(*block, context);
}
void Value::mkPath(const SourcePath & path)
lix/libexpr/value.hh
+72 -49
View File
@@ -7,8 +7,10 @@
#include <cstring>
#include <functional>
#include <limits>
#include <memory>
#include <ranges>
#include <span>
#include <string_view>
#include <type_traits>
#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<Str *>(LIX_GC_MALLOC_ATOMIC(sizeof(Value::Str) + size));
if (result) {
result->length = size;
return result;
}
throw std::bad_alloc();
}
static std::unique_ptr<Str, Deleter> 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<Str *>(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<String>();
*block = {.content = strPtr, .context = contextPtr};
@@ -394,7 +457,7 @@ public:
Value(string_t, std::string_view copyFrom, NixStringContext const & context = {})
{
auto block = gcAllocType<String>();
*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<String>();
*block = {.content = strPtr, .context = nullptr};
raw = tag(tString, block);
if (context.empty()) {
// It stays nullptr
return;
}
// Copy the context.
block->context = gcAllocType<char const *>(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<String>();
*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<path>", 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<String>();
*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<String>();
*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<const String *>()->isPath());
return SourcePath{CanonPath(untag<const String *>()->content)};
return SourcePath{CanonPath(untag<const String *>()->content->str())};
}
std::string_view str() const
{
assert(internalType() == tString && !untag<const String *>()->isPath());
return std::string_view(untag<const String *>()->content);
return std::string_view(untag<const String *>()->content->str());
}
NixInt integer() const
lix/libutil/escape-string.cc
+1 -1
View File
@@ -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};
tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromJSON-value.out.exp
BIN
View File
Binary file not shown.
tests/functional2/lang/nul_bytes_parsers/eval-allow-depr-fromTOML-value.out.exp
BIN
View File
Binary file not shown.
tests/functional2/lang/nul_bytes_parsers/eval-depr-fromJSON-value.out.exp
BIN
View File
Binary file not shown.
tests/functional2/lang/nul_bytes_parsers/eval-depr-fromTOML-value.out.exp
BIN
View File
Binary file not shown.
tests/unit/libexpr-support/tests/libexpr.hh
+1 -1
View File
@@ -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;