From 8d82b13dad37d1eca00067cb0834d29f344ea762 Mon Sep 17 00:00:00 2001 From: deeaitch Date: Mon, 5 Oct 2026 18:33:19 -0400 Subject: [PATCH] Closes #6 --- README.md | 38 ++++++++-- dbc_statement_handler.cpp | 143 ++++++++++++++------------------------ issue-sg-validation.md | 40 +++++++++++ tests/README.md | 3 + tests/dbc_parser_test.cpp | 97 ++++++++++++++++++++++++++ 5 files changed, 227 insertions(+), 94 deletions(-) create mode 100644 issue-sg-validation.md diff --git a/README.md b/README.md index 6984da4..c0b69a0 100644 --- a/README.md +++ b/README.md @@ -532,9 +532,8 @@ one-based physical line number, including blank lines: example.dbc:3: Invalid CAN ID: invalid ``` -This change validates frame definitions. Signal syntax and unsupported DBC -constructs retain their existing behavior; comprehensive validation is still -planned. File-open errors contain the path but have no line number. +This change validates frame definitions. Signal definitions are also validated as described below; comprehensive +DBC grammar coverage is still planned. File-open errors contain the path but have no line number. --- @@ -562,6 +561,35 @@ Parsed fields: --- +### Signal validation and parse errors + +`SG_` statements are matched completely. Signal names must be nonempty and +contain no whitespace. Spaces and tabs are accepted between syntax elements; +byte order and signedness remain adjacent (`@0+`, `@0-`, `@1+`, or `@1-`). +A quoted unit (which may be empty) and a nonempty comma-separated receiver +list are required. Unexpected trailing fields are rejected. + +Start bits must be unsigned decimal integers fitting `uint32_t`; lengths must +be within `1..64` bits. Signs, fractional values, numeric suffixes, and overflow +are rejected for these fields. Every signal bit must fit within the frame's +declared payload length, using Intel increasing bit positions or Motorola +sawtooth traversal. This uses the stored payload length without introducing +an additional CAN or CAN FD payload limit. + +Factor, offset, minimum, and maximum accept signed decimal numbers, fractions, +and scientific notation. The entire numeric token must be valid; hexadecimal +numbers, NaN, infinity, conversion overflow, and conversion underflow are +rejected. Minimum must not exceed maximum. Zero and negative factors are allowed. + +Multiplexing markers (`M`, `mN`, and combinations) are currently rejected rather +than interpreted as part of a signal name. Multiplexing support and duplicate +frame/signal detection remain separate work. + +Invalid signal definitions stop parsing with `path:line: description`, including +when the signal exceeds the declared payload length. + +--- + ## Comments Frame comment example: @@ -805,8 +833,8 @@ RegisterType("VAL_"); // after implementing DbcValueHandler Registration creates one handler; replacing a registration replaces that instance. Register during initialization before dispatching. A line is routed using its full first whitespace-delimited keyword, so `BO_TX_BU_` cannot match `BO_`. -Unregistered keywords remain ignored. Existing `SG_` and `CM_` syntax policies -are preserved, including requiring a literal space after those keywords. +Unregistered keywords remain ignored. `SG_` accepts spaces and tabs after its keyword; `CM_` retains its existing +policy requiring a literal space. This factory handles statements inside DBC files. A separate format-level factory can be introduced when another file format is implemented. The decoder diff --git a/dbc_statement_handler.cpp b/dbc_statement_handler.cpp index 293caa7..cfda956 100644 --- a/dbc_statement_handler.cpp +++ b/dbc_statement_handler.cpp @@ -5,6 +5,8 @@ #include #include #include +#include +#include namespace { std::uint32_t ParseDecimal (const std::string &text, @@ -142,95 +144,50 @@ FrameInfo DbcFrameHandler::ParseFrameLine (const std::string &line) { } SignalInfo DbcSignalHandler::ParseSignalLine (const std::string &line) { - /* - * Example: - * SG_ EngineSpeed : 0|16@1+ (0.125,0) [0|8000] "rpm" ECU1,ECU2 - */ + // Match the complete statement before converting any numeric fields. + static const std::regex syntax ( + R"dbc(^SG_[ \t]+([^:]+):[ \t]*([^| \t]+)[ \t]*\|[ \t]*([^@ \t]+)[ \t]*@[ \t]*([01])([+-])[ \t]*\([ \t]*([^, \t]+)[ \t]*,[ \t]*([^ )\t]+)[ \t]*\)[ \t]*\[[ \t]*([^| \t]+)[ \t]*\|[ \t]*([^\] \t]+)[ \t]*\][ \t]*"([^"\r\n]*)"[ \t]+([^ \t,]+(?:[ \t]*,[ \t]*[^ \t,]+)*)[ \t]*$)dbc"); + std::smatch fields; + if (!std::regex_match (line, fields, syntax)) + throw std::runtime_error ("Malformed or unsupported SG_ signal definition."); SignalInfo signal; + signal.name = TrimText (fields[1].str()); + if (signal.name.empty() || signal.name.find_first_of (" \t\r\n") != std::string::npos) + throw std::runtime_error ("Invalid signal name or unsupported multiplexing syntax."); - std::string work = TrimText (line); - if (work.compare (0U, 4U, "SG_ ") != 0) - throw std::runtime_error ("Invalid signal line: " + line); - - work.erase (0U, 4U); - - const std::string::size_type colonPos = work.find (':'); - if (colonPos == std::string::npos) - throw std::runtime_error ("Signal line missing ':' : " + line); - - signal.name = TrimText (work.substr (0U, colonPos)); - std::string rest = TrimText (work.substr (colonPos + 1U)); - - const std::string::size_type pipePos = rest.find ('|'); - const std::string::size_type atPos = rest.find ('@'); - const std::string::size_type signPos = rest.find_first_of ("+-", atPos); - const std::string::size_type factorBegin = rest.find ('('); - const std::string::size_type factorComma = rest.find (',', factorBegin); - const std::string::size_type factorEnd = rest.find (')', factorComma); - const std::string::size_type rangeBegin = rest.find ('['); - const std::string::size_type rangeSep = rest.find ('|', rangeBegin); - const std::string::size_type rangeEnd = rest.find (']', rangeSep); - const std::string::size_type unitBegin = rest.find ('"', rangeEnd); - const std::string::size_type unitEnd = rest.find ('"', unitBegin + 1U); - - if ((pipePos == std::string::npos) || - (atPos == std::string::npos) || - (signPos == std::string::npos) || - (factorBegin == std::string::npos) || - (factorComma == std::string::npos) || - (factorEnd == std::string::npos) || - (rangeBegin == std::string::npos) || - (rangeSep == std::string::npos) || - (rangeEnd == std::string::npos) || - (unitBegin == std::string::npos) || - (unitEnd == std::string::npos)) - throw std::runtime_error ("Unsupported signal syntax: " + line); - - signal.startBit = static_cast ( - std::stoul (TrimText (rest.substr (0U, pipePos))) - ); - - signal.length = static_cast ( - std::stoul (TrimText (rest.substr (pipePos + 1U, atPos - pipePos - 1U))) - ); - - { - if ((atPos + 1U) >= rest.size()) - throw std::runtime_error ("Invalid endianness in signal: " + line); - - const char endianChar = rest[atPos + 1U]; - signal.isLittleEndian = (endianChar == '1'); - } - - { - const char signChar = rest[signPos]; - signal.isSigned = (signChar == '-'); - } - - signal.factor = std::stod ( - TrimText (rest.substr (factorBegin + 1U, factorComma - factorBegin - 1U)) - ); - - signal.offset = std::stod ( - TrimText (rest.substr (factorComma + 1U, factorEnd - factorComma - 1U)) - ); - - signal.minimum = std::stod ( - TrimText (rest.substr (rangeBegin + 1U, rangeSep - rangeBegin - 1U)) - ); - - signal.maximum = std::stod ( - TrimText (rest.substr (rangeSep + 1U, rangeEnd - rangeSep - 1U)) - ); - - signal.unit = rest.substr (unitBegin + 1U, unitEnd - unitBegin - 1U); - - { - const std::string receiversText = TrimText (rest.substr (unitEnd + 1U)); - signal.receivers = SplitReceivers (receiversText); - } + signal.startBit = ParseDecimal (fields[2].str(), + std::numeric_limits::max(), "signal start bit"); + signal.length = ParseDecimal (fields[3].str(), 64U, "signal length"); + if (signal.length == 0U) + throw std::runtime_error ("Signal length must be within 1..64 bits."); + signal.isLittleEndian = fields[4].str() == "1"; + signal.isSigned = fields[5].str() == "-"; + const auto parsePhysical = [] (const std::string &text, const char *field) { + static const std::regex number ( + R"(^[+-]?(?:[0-9]+(?:\.[0-9]*)?|\.[0-9]+)(?:[eE][+-]?[0-9]+)?$)"); + if (!std::regex_match (text, number)) + throw std::runtime_error (std::string ("Invalid ") + field + ": " + text); + std::size_t consumed = 0U; + double value; + try { + value = std::stod (text, &consumed); + } catch (const std::exception &) { + throw std::runtime_error (std::string ("Invalid ") + field + ": " + text); + } + if (consumed != text.size() || !std::isfinite (value)) + throw std::runtime_error (std::string ("Invalid ") + field + ": " + text); + return value; + }; + signal.factor = parsePhysical (fields[6].str(), "signal factor"); + signal.offset = parsePhysical (fields[7].str(), "signal offset"); + signal.minimum = parsePhysical (fields[8].str(), "signal minimum"); + signal.maximum = parsePhysical (fields[9].str(), "signal maximum"); + if (signal.minimum > signal.maximum) + throw std::runtime_error ("Signal minimum exceeds maximum."); + signal.unit = fields[10].str(); + signal.receivers = SplitReceivers (fields[11].str()); return signal; } @@ -326,12 +283,20 @@ void DbcFrameHandler::Handle (const std::string &line, DbcParseContext &context) } void DbcSignalHandler::Handle (const std::string &line, DbcParseContext &context) const { - // Preserve the existing signal syntax policy during this refactor. - if (line.compare (0U, 4U, "SG_ ") != 0) - return; if (context.currentFrame == nullptr) throw std::runtime_error ("Signal found before any frame definition."); - context.currentFrame->signals.push_back (ParseSignalLine (line)); + SignalInfo signal = ParseSignalLine (line); + const std::uint64_t payloadBits = static_cast (context.currentFrame->dlc) * 8U; + std::uint64_t bit = signal.startBit; + for (std::uint32_t index = 0U; index < signal.length; ++index) { + if (bit >= payloadBits) + throw std::runtime_error ("Signal exceeds the frame payload length."); + if (signal.isLittleEndian) + ++bit; + else + bit = (bit % 8U == 0U) ? bit + 15U : bit - 1U; + } + context.currentFrame->signals.push_back (std::move (signal)); } void DbcCommentHandler::Handle (const std::string &line, DbcParseContext &context) const { diff --git a/issue-sg-validation.md b/issue-sg-validation.md new file mode 100644 index 0000000..62062f9 --- /dev/null +++ b/issue-sg-validation.md @@ -0,0 +1,40 @@ +# Validate `SG_` signal definitions and report malformed or unsupported syntax + +## Description + +Following strict validation of `BO_` frame definitions, add equivalent validation for `SG_` signal definitions. Currently, some malformed statements are accepted silently, and unsupported syntax can produce incorrect decoding. + +Examples of current behavior: + +- `@2+` is interpreted as Motorola instead of being rejected. +- Numeric fields containing suffixes, such as `0junk` or `1junk`, are partially parsed and accepted. +- `SG_ Value m1 : ...` is treated as an ordinary signal named `Value m1`, without multiplexing conditions. +- A statement containing a tab after `SG_` is silently skipped. + +## Scope + +- Validate statement structure and require a nonempty signal name without whitespace. +- Accept spaces and tabs between syntax elements. +- Parse numeric fields completely, rejecting overflow and trailing characters; preserve support for fractional values and scientific notation in physical parameters. +- Accept only byte order `@0` or `@1` and signedness `+` or `-`. +- Require signal lengths within `1..64` bits. +- Validate that the signal fits within the frame's declared payload length, using the appropriate Intel or Motorola bit traversal. +- Reject NaN and infinite values for factor, offset, minimum, and maximum; require `minimum <= maximum`. +- Explicitly reject multiplexing markers `M`, `mN`, and their combinations until multiplexing is implemented. +- Preserve the error format `path:line: description`. + +## Acceptance criteria + +- [ ] Existing valid signal definitions continue to parse successfully. +- [ ] Malformed and unsupported definitions stop parsing with a clear error. +- [ ] Regression tests cover the cases above, numeric boundaries, and Intel/Motorola payload boundaries. +- [ ] Existing parser and decoder tests pass. +- [ ] README documents the new validation rules and limitations. + +## Out of scope + +Duplicate detection, multiplexing support, and `VAL_` support remain separate tasks. + +## Suggested branch + +`codex/validate-sg-definitions` diff --git a/tests/README.md b/tests/README.md index d23afc7..8cb7c75 100644 --- a/tests/README.md +++ b/tests/README.md @@ -43,6 +43,9 @@ following the decoder instructions above, then run `./dbc_parser_test`. Parser tests cover standard and extended CAN IDs, whitespace, payload storage boundaries, missing fields, malformed and overflowing numbers, reserved ID bits, error paths and line numbers, and integration with signals and comments. +Signal tests cover complete syntax and numeric validation, scientific notation, +finite physical parameters and ordered ranges, spaces and tabs, receiver lists, +unsupported multiplexing, and Intel/Motorola declared-payload boundaries. Temporary DBC fixtures are created in the system temporary directory and removed automatically. diff --git a/tests/dbc_parser_test.cpp b/tests/dbc_parser_test.cpp index 5bee06b..b27f9a1 100644 --- a/tests/dbc_parser_test.cpp +++ b/tests/dbc_parser_test.cpp @@ -100,6 +100,103 @@ int main() { check (std::string (error.what()).find (fixture.path.string() + ":2: ") == 0U, "Signal error location"); } + const DbcParser signalParser; + const auto signalLine = [] (const std::string &start, const std::string &length, + const std::string &order, const std::string &factor, + const std::string &offset, const std::string &minimum, + const std::string &maximum) { + return "SG_ Value : " + start + "|" + length + "@" + order + + " (" + factor + "," + offset + ") [" + minimum + "|" + maximum + "] \"\" ECU"; + }; + const auto rejectSignal = [&] (const std::string &line, unsigned payload = 8U) { + fixture.Write ("BO_ 1 Frame: " + std::to_string (payload) + " ECU\n\n" + line + "\n"); + try { + signalParser.ParseFile (fixture.path.string()); + check (false, "Reject " + line); + } catch (const std::runtime_error &error) { + const std::string prefix = fixture.path.string() + ":3: "; + check (std::string (error.what()).find (prefix) == 0U && + std::string (error.what()).size() > prefix.size(), "Reject " + line); + } + }; + const std::string ordinary = signalLine ("0", "8", "1+", "1", "0", "0", "255"); + for (const std::string value : {"-1", "+1", "0junk", "0x10", "1.5", + "4294967296", "999999999999999999999"}) { + rejectSignal (signalLine (value, "8", "1+", "1", "0", "0", "255")); + rejectSignal (signalLine ("0", value, "1+", "1", "0", "0", "255")); + } + for (const std::string length : {"0", "65"}) + rejectSignal (signalLine ("0", length, "1+", "1", "0", "0", "255")); + for (const std::string order : {"2+", "9-", "1", "1x", "1++", "01+"}) + rejectSignal (signalLine ("0", "8", order, "1", "0", "0", "255")); + for (const std::string value : {"1junk", "nan", "NaN", "inf", "-inf", "1e999", "1e-999", "0x1p2", "", "1e", "1.2.3"}) { + rejectSignal (signalLine ("0", "8", "1+", value, "0", "0", "255")); + rejectSignal (signalLine ("0", "8", "1+", "1", value, "0", "255")); + rejectSignal (signalLine ("0", "8", "1+", "1", "0", value, "255")); + rejectSignal (signalLine ("0", "8", "1+", "1", "0", "0", value)); + } + rejectSignal (signalLine ("0", "8", "1+", "1", "0", "2", "1")); + for (const std::string name : {"", "Two Names", "Value M", "Value m1", "Value m1M"}) { + std::string line = ordinary; + line.replace (4U, 5U, name); + rejectSignal (line); + } + for (const std::string broken : { + "SG_", "SG_ Value", "SG_ Value : 0|8@1+ (1,0) [0|255] ECU", + "SG_ Value : 0|8@1+ (1,0) [0|255] \"\"", + "SG_ Value : 0|8@1+ (1,0) [0|255] \"\" ECU,", + "SG_ Value : 0|8@1+ (1,0) [0|255] \"\" ECU extra"}) + rejectSignal (broken); + rejectSignal (signalLine ("64", "1", "1+", "1", "0", "0", "1")); + rejectSignal (signalLine ("63", "2", "1+", "1", "0", "0", "3")); + rejectSignal (signalLine ("56", "2", "0+", "1", "0", "0", "3")); + rejectSignal (signalLine ("4294967295", "64", "1+", "1", "0", "0", "1")); + rejectSignal (ordinary, 0U); + for (const char delimiter : {'|', '@', '(', ')', '[', ']', '"'}) { + std::string line = ordinary; + line.erase (line.find (delimiter), 1U); + rejectSignal (line); + } + fixture.Write ("BO_ 1 Frame: 8 ECU\n" + + signalLine ("0", "8", "1+", "0", "0", "0", "0") + "\n" + + signalLine ("8", "8", "1-", "-1", "0", "-255", "0") + "\n"); + const auto factors = signalParser.ParseFile (fixture.path.string()); + check (factors.frames[0].signals.size() == 2U && + factors.frames[0].signals[0].factor == 0.0 && + factors.frames[0].signals[1].factor == -1.0, + "Zero and negative factors and equal range boundaries"); + + struct SignalCase { const char *start; const char *length; const char *order; unsigned payload; }; + const SignalCase validSignals[] = { + {"0", "1", "1+", 1U}, {"7", "1", "0-", 1U}, + {"0", "64", "1+", 8U}, {"7", "64", "0-", 8U}, + {"63", "1", "1+", 8U}, {"56", "1", "0+", 8U}, + {"3", "12", "0+", 2U}, {"2039", "1", "1+", 255U} + }; + for (const auto &test : validSignals) { + fixture.Write ("BO_ 1 Frame: " + std::to_string (test.payload) + " ECU\n" + + signalLine (test.start, test.length, test.order, ".5e+1", "-2.5E-1", "-1.", "+2e2") + "\n"); + try { + const auto parsed = signalParser.ParseFile (fixture.path.string()); + const auto &signal = parsed.frames.at (0).signals.at (0); + check (signal.startBit == std::stoul (test.start) && + signal.length == std::stoul (test.length) && + signal.isLittleEndian == (test.order[0] == '1') && + signal.isSigned == (test.order[1] == '-') && + signal.factor == 5.0 && signal.offset == -0.25 && + signal.minimum == -1.0 && signal.maximum == 200.0, + "Valid signal boundary and numeric metadata"); + } catch (const std::exception &error) { check (false, error.what()); } + } + fixture.Write ("BO_ 1 Frame: 8 ECU\n" + "SG_\tValue\t:\t0 | 8 @1+ ( 1 , 0 ) [ 0 | 255 ] \"rpm\" ECU1,\tECU2\n"); + try { + const auto parsed = signalParser.ParseFile (fixture.path.string()); + const auto &signal = parsed.frames.at (0).signals.at (0); + check (signal.name == "Value" && signal.unit == "rpm" && + signal.receivers == std::vector ({"ECU1", "ECU2"}), + "Signal tabs, spaces, and receiver list"); + } catch (const std::exception &error) { check (false, error.what()); } DbcDatabase dispatchDatabase; DbcParseContext context {dispatchDatabase, nullptr}; DbcStatementDispatcher dispatcher;