Skip to content

Commit 999883a

Browse files
committed
fix: reject/clamp LDF signal bit_width > 64 to prevent UB in decode()
lin::ldf::DB::decode() shifts a uint64_t by up to bit_width - 1 bits (`1ULL << i`) with no upper bound on the LDF-declared bit_width. LDF signals with bit_width > 64 (unconstrained int, no range check in parse_signals()) trigger a shift-by->=64, which is undefined behavior and aborts under UBSan. Reaching the actual UB requires decode()'s data argument to exceed 8 bytes, which is a normal, unbounded std::vector<uint8_t>& — a malformed or crafted .ldf file can therefore crash any process that parses it and later decodes an oversized frame against it. Fixes: - parse_signals() now rejects (skips) any signal whose parsed bit_width falls outside [1, 64], consistent with how the parser already skips other malformed signal lines rather than storing bad data. - decode() additionally clamps its own loop bound to [0, 64] as defense in depth, since DB's storage members are public and a Signal can be constructed directly by a caller that bypasses the text parser entirely. Closes #18 Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com>
1 parent b2480f4 commit 999883a

2 files changed

Lines changed: 84 additions & 1 deletion

File tree

src/ldf/parser.cpp

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,13 @@ DB::decode(uint8_t id, const std::vector<uint8_t>& data) const {
6060

6161
// LSB-first (Intel) bit extraction — REQ-LDF-009
6262
uint64_t val = 0;
63-
int bit_width = sit->second.bit_width;
63+
// Defense-in-depth clamp: parse_signals() already rejects
64+
// out-of-range bit_width at parse time, but decode() must not rely
65+
// solely on that — a Signal can also be constructed directly by
66+
// callers who bypass the LDF text parser. `1 << i` for i >= 64 is
67+
// undefined behavior (aborts under UBSan); clamp the loop bound
68+
// itself rather than trusting the stored value.
69+
int bit_width = std::clamp(sit->second.bit_width, 0, 64);
6470
for (int i = 0; i < bit_width; ++i) {
6571
int byte_idx = (ref.bit_offset + i) / 8;
6672
int bit_idx = (ref.bit_offset + i) % 8;
@@ -193,6 +199,13 @@ struct Parser {
193199
Signal sig;
194200
sig.name = name;
195201
try { sig.bit_width = static_cast<int>(parse_int(parts[0])); } catch (...) {}
202+
// A bit_width outside [1, 64] cannot be decoded (decode()'s bit
203+
// extraction loop shifts a uint64_t by `i` bits) and LIN frames
204+
// are at most kLINMaxDataLen (8) bytes = 64 bits wide anyway, so
205+
// treat it the same as any other malformed signal line: skip it
206+
// rather than storing a Signal that would later cause undefined
207+
// behavior (shift-by->=64) in decode(). fusa:req REQ-LDF-008
208+
if (sig.bit_width < 1 || sig.bit_width > 64) continue;
196209
try { sig.init_value = parse_uint(parts[1]); } catch (...) {}
197210
sig.publisher = trim(parts[2]);
198211
for (std::size_t i = 3; i < parts.size(); ++i) {

tests/test_ldf.cpp

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,3 +190,73 @@ TEST_CASE("Frames() returns defensive copy", "[ldf][REQ-LDF-015]") {
190190
frames.clear();
191191
CHECK(db->frame(0x10) != nullptr);
192192
}
193+
194+
TEST_CASE("parse rejects signal bit_width > 64", "[ldf][REQ-LDF-007]") {
195+
// BigSignal's declared bit_width (128) exceeds what any uint64_t-based
196+
// decode() can represent (and exceeds 8*kLINMaxDataLen bits anyway) —
197+
// parse_signals() must not store it, or a later decode() call against a
198+
// >8-byte buffer would shift a uint64_t by >=64 bits (UB).
199+
static const char* kLDF = R"(
200+
Signals {
201+
BigSignal : 128, 0, MotorControl, BCM ;
202+
MotorSpeed : 8, 0, MotorControl, BCM ;
203+
}
204+
205+
Frames {
206+
BigFrame : 0x30, MotorControl, 10 {
207+
BigSignal, 0 ;
208+
}
209+
}
210+
)";
211+
std::istringstream ss(kLDF);
212+
auto db = parse(ss);
213+
REQUIRE(db != nullptr);
214+
CHECK(db->signal("BigSignal") == nullptr);
215+
CHECK(db->signal("MotorSpeed") != nullptr);
216+
217+
// The frame itself still parses; decoding it against a >8-byte buffer
218+
// (the precondition needed to actually reach the unclamped shift) must
219+
// not crash, and the unresolvable signal is simply absent from the
220+
// result rather than UB.
221+
std::vector<uint8_t> data(10, 0xFF);
222+
std::unordered_map<std::string, uint64_t> result;
223+
REQUIRE_NOTHROW(result = db->decode(0x30, data));
224+
CHECK(result.count("BigSignal") == 0);
225+
}
226+
227+
TEST_CASE("parse rejects signal bit_width == 0", "[ldf][REQ-LDF-007]") {
228+
static const char* kLDF = R"(
229+
Signals {
230+
ZeroWidth : 0, 0, MotorControl, BCM ;
231+
}
232+
)";
233+
std::istringstream ss(kLDF);
234+
auto db = parse(ss);
235+
REQUIRE(db != nullptr);
236+
CHECK(db->signal("ZeroWidth") == nullptr);
237+
}
238+
239+
TEST_CASE("Decode clamps an out-of-range bit_width instead of UB", "[ldf][REQ-LDF-009]") {
240+
// Bypasses the text parser entirely (DB's storage members are public,
241+
// populated by parse() but constructible directly by any caller) to
242+
// exercise decode()'s own defense-in-depth clamp, independent of the
243+
// parse-time rejection covered above.
244+
DB db;
245+
Signal sig;
246+
sig.name = "Huge";
247+
sig.bit_width = 200;
248+
db.signals_["Huge"] = sig;
249+
250+
LDFFrame fr;
251+
fr.name = "HugeFrame";
252+
fr.id = 0x31;
253+
fr.signals.push_back(SignalRef{"Huge", 0});
254+
db.frames_[0x31] = fr;
255+
256+
std::vector<uint8_t> data(20, 0xFF); // > 8 bytes: reaches the clamp path
257+
std::unordered_map<std::string, uint64_t> result;
258+
REQUIRE_NOTHROW(result = db.decode(0x31, data));
259+
REQUIRE(result.count("Huge") == 1);
260+
// bit_width clamped to 64; with all-0xFF input every extracted bit is 1.
261+
CHECK(result.at("Huge") == 0xFFFFFFFFFFFFFFFFULL);
262+
}

0 commit comments

Comments
 (0)