Skip to content

Commit

Permalink
Fix incorrect data consumption for &max-size.
Browse files Browse the repository at this point in the history
We would previously handle `&size` and `&max-size` almost identical
with the only difference that `&max-size` sets up a slightly larger view
to accomodate a sentinel. In particular, we also used identical code to
set up the position where parsing should resume after such a field.

This was incorrect as it is in general impossible to tell where parsing
continues after a field with `&max-size` since it does not signify a
fixed view like `&size`. In this patch we now compute the next position
for a `&max-size` field by inspecting the limited view to detect how
much data was extracted.

Closes #1668.
  • Loading branch information
bbannier committed Feb 5, 2024
1 parent 041a072 commit 8044db2
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 10 deletions.
16 changes: 15 additions & 1 deletion spicy/toolchain/src/compiler/codegen/parser-builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,11 @@ struct ProductionVisitor

pb->parseError("parsing not done within &max-size bytes", a->meta());
});

// For `&max-size` we use `ncur` to store a view limited and slightly larger than the _permissible_ range,
// not the _consumed_ range. Update the value with where we ended up during parsing so we can inspect it
// below when setting the position after parsing.
ncur = state().cur;
}

else if ( auto a = AttributeSet::find(field->attributes(), "&size") ) {
Expand Down Expand Up @@ -814,8 +819,17 @@ struct ProductionVisitor
pb->saveParsePosition();
}

if ( ncur )
if ( ncur ) {
// For `&max-size` we use `ncur` to store a view limited and slightly larger than the _permissible_ range,
// not the _consumed_ range. We need to manually compute the actually consumed range from where we ended
// parsing in the limited view. We update the value stored in `ncur` so it can be updated below.
if ( AttributeSet::find(field->attributes(), "&max-size") )
ncur = builder::memberCall(state().cur, "advance",
{builder::difference(builder::memberCall(*ncur, "offset", {}),
builder::memberCall(state().cur, "offset", {}))});

builder()->addAssign(state().cur, *ncur);
}

if ( ! meta.container() ) {
if ( pb->isEnabledDefaultNewValueForField() && state().literal_mode == LiteralMode::Default )
Expand Down
4 changes: 2 additions & 2 deletions tests/Baseline/spicy.types.unit.field-max-size/output
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
done, [$xs=b"\x00"]
done, [$xs=b"\x01\x00"]
error, [$xs=(not set)]
[error] terminating with uncaught exception of type spicy::rt::ParseError: parsing not done within &max-size bytes (<...>/field-max-size.spicy:15:40-15:56)
[error] terminating with uncaught exception of type spicy::rt::ParseError: parsing not done within &max-size bytes (<...>/field-max-size.spicy:16:40-16:56)
error, [$xs=b"\x01\x01\x01"]
[error] terminating with uncaught exception of type spicy::rt::ParseError: end-of-data reached before &until-including expression found (<...>/field-max-size.spicy:15:32-15:38)
[error] terminating with uncaught exception of type spicy::rt::ParseError: end-of-data reached before &until-including expression found (<...>/field-max-size.spicy:16:32-16:38)
11 changes: 11 additions & 0 deletions tests/Baseline/spicy.types.unit.field-max-size/output2
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
Mini::X {
use_max_size: True
xs:
rest: BC
}
Mini::X {
use_max_size: False
xs:
rest: BC
}
31 changes: 24 additions & 7 deletions tests/spicy/types/unit/field-max-size.spicy
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
# @TEST-EXEC: spicyc -j -d %INPUT -o test.hlto
# @TEST-EXEC: printf '\000' | spicy-driver -d test.hlto >>output 2>&1
# @TEST-EXEC: printf '\001\000' | spicy-driver -d test.hlto >>output 2>&1
# @TEST-EXEC-FAIL: printf '\001\001\000' | spicy-driver -d test.hlto >>output 2>&1
# @TEST-EXEC-FAIL: printf '\001\001\001\000' | spicy-driver -d test.hlto >>output 2>&1
# @TEST-EXEC: btest-diff output
#
# @TEST-DOC: Validate effects of field-level `&max-size` attribute
#
# @TEST-EXEC: spicyc -j -d %INPUT -o test.hlto

module Mini;

# @TEST-EXEC: printf '\000' | spicy-driver -d test.hlto -p Mini::Test >>output 2>&1
# @TEST-EXEC: printf '\001\000' | spicy-driver -d test.hlto -p Mini::Test >>output 2>&1
# @TEST-EXEC-FAIL: printf '\001\001\000' | spicy-driver -d test.hlto -p Mini::Test >>output 2>&1
# @TEST-EXEC-FAIL: printf '\001\001\001\000' | spicy-driver -d test.hlto -p Mini::Test >>output 2>&1
# @TEST-EXEC: btest-diff output

const MaxSize = 2;

public type Test = unit {
Expand All @@ -17,3 +18,19 @@ public type Test = unit {
on %done { print "done", self; }
on %error { print "error", self; }
};

# Check that `&max-size` has no effect on how much data is consumed. This is a regression test for #1668.
# @TEST-EXEC: printf '\001_BC' | spicy-dump -d test.hlto -p Mini::X >>output2 2>&1
# @TEST-EXEC: printf '\000_BC' | spicy-dump -d test.hlto -p Mini::X >>output2 2>&1
# @TEST-EXEC: btest-diff output2

public type X = unit {
use_max_size: uint8 &convert=cast<bool>($$);

switch (self.use_max_size) {
True -> xs: bytes &until=b"_" &max-size=1;
False -> xs: bytes &until=b"_";
};

rest: bytes &eod;
};

0 comments on commit 8044db2

Please sign in to comment.