llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Yao Qi (qiyao) <details> <summary>Changes</summary> The DIL bitfield extraction operator `base[high:low]` creates a synthetic bitfield child without validating the requested range. Three malformed ranges reach the data layer and either return nonsense or crash. Reproduced with a 32-bit `int value` and DIL enabled: ``` (lldb) settings set target.experimental.use-DIL true (lldb) frame variable 'value[-1:0]' (int:2) value[-1:0] = 2 ``` A negative index is accepted and produces a meaningless child. `first_index`/`last_index` are signed `int64_t`, but `GetSyntheticBitFieldChild` takes `uint32_t`, so `-1` silently wraps to a huge unsigned offset. ``` (lldb) frame variable 'value[0:64]' Assertion failed: (bitfield_bit_size <= 64), function GetMaxU64Bitfield, file DataExtractor.cpp, line 580. ``` A width greater than 64 bits aborts. `DataExtractor::GetMaxU64Bitfield` only supports up to 64 bits: it asserts in an assertions build and otherwise performs an out-of-bounds shift. A 32-bit `value` with range `[0:64]` is 65 bits, enough to trip it. ``` (lldb) frame variable 'value[100:50]' (int:51) value[100:50] = 0 ``` A high index past the base object's storage returns a garbage child in a normal build. Under UBSan the read/format path shifts by an oversized amount derived from the offset: ``` (lldb) frame variable 'value[100:50]' DataExtractor.cpp:591:12: runtime error: shift exponent 234 is too large for 64-bit type 'uint64_t' ``` Reject all three in the DIL evaluator before the synthetic child is created: a negative `first_index`/`last_index`, a normalized width greater than 64 bits, and a high index at or beyond the base object's bit size (queried with `GetCompilerType().GetBitSize`). Each returns a `DILDiagnosticError` with a clear message. Valid in-range extractions are unaffected. Adds the three malformed ranges to the DIL bitfield extraction API test (`TestFrameVarDILBitFieldExtraction`). Without the fix the test fails on the first case (`value[-1:0]` is expected to error but succeeds); the `[0:64]` case additionally asserts and the `[100:50]` case is a UBSan shift-out-of-bounds. --- Full diff: https://github.com/llvm/llvm-project/pull/213055.diff 2 Files Affected: - (modified) lldb/source/ValueObject/DILEval.cpp (+38) - (modified) lldb/test/API/commands/frame/var-dil/basics/BitFieldExtraction/TestFrameVarDILBitFieldExtraction.py (+34) ``````````diff diff --git a/lldb/source/ValueObject/DILEval.cpp b/lldb/source/ValueObject/DILEval.cpp index 4c5ac96dccf74..7e443419bee2d 100644 --- a/lldb/source/ValueObject/DILEval.cpp +++ b/lldb/source/ValueObject/DILEval.cpp @@ -1277,14 +1277,52 @@ Interpreter::Visit(const BitFieldExtractionNode &node) { return llvm::make_error<DILDiagnosticError>( m_expr, "could not get the index as an integer", node.GetLocation()); + if (first_index < 0 || last_index < 0) { + std::string message = + llvm::formatv("bitfield range {0}:{1} is not valid (negative index)", + first_index, last_index); + return llvm::make_error<DILDiagnosticError>(m_expr, message, + node.GetLocation()); + } + // if the format given is [high-low], swap range if (first_index > last_index) std::swap(first_index, last_index); + // The underlying DataExtractor bitfield accessors only support extracting up + // to 64 bits at once (GetMaxU64Bitfield asserts bitfield_bit_size <= 64 and + // would otherwise perform an out-of-bounds shift). Reject oversized ranges + // here instead of crashing deep in the data layer. + if (last_index - first_index + 1 > 64) { + std::string message = + llvm::formatv("bitfield range {0}:{1} is not valid (more than 64 bits)", + first_index, last_index); + return llvm::make_error<DILDiagnosticError>(m_expr, message, + node.GetLocation()); + } + auto base_or_err = EvaluateAndDereference(node.GetBase()); if (!base_or_err) return base_or_err; lldb::ValueObjectSP base = *base_or_err; + + // The bitfield range must lie within the storage of the base object. A + // bit offset/size that extends past the base's bit size leads to an + // out-of-bounds shift when the value is later read or formatted (e.g. in + // DataExtractor::GetMaxU64Bitfield via DumpDataExtractor). + llvm::Expected<uint64_t> base_bit_size = + base->GetCompilerType().GetBitSize(&m_stack_frame); + if (!base_bit_size) + return base_bit_size.takeError(); + if (static_cast<uint64_t>(last_index) >= *base_bit_size) { + std::string message = llvm::formatv( + "bitfield range {0}:{1} is not valid for \"({2}) {3}\"", first_index, + last_index, base->GetTypeName().AsCString("<invalid type>"), + base->GetName().GetStringRef()); + return llvm::make_error<DILDiagnosticError>(m_expr, message, + node.GetLocation()); + } + lldb::ValueObjectSP child_valobj_sp = base->GetSyntheticBitFieldChild(first_index, last_index, true); if (!child_valobj_sp) { diff --git a/lldb/test/API/commands/frame/var-dil/basics/BitFieldExtraction/TestFrameVarDILBitFieldExtraction.py b/lldb/test/API/commands/frame/var-dil/basics/BitFieldExtraction/TestFrameVarDILBitFieldExtraction.py index 026658c0cd25f..11dce1e494ead 100644 --- a/lldb/test/API/commands/frame/var-dil/basics/BitFieldExtraction/TestFrameVarDILBitFieldExtraction.py +++ b/lldb/test/API/commands/frame/var-dil/basics/BitFieldExtraction/TestFrameVarDILBitFieldExtraction.py @@ -66,3 +66,37 @@ def test_bitfield_extraction(self): error=True, substrs=["bit index is not an integer"], ) + + # A negative bit index must be rejected with a clear error instead of + # silently wrapping to a huge uint32_t at the GetSyntheticBitFieldChild + # call site. + self.expect( + "frame var 'value[-1:0]'", + error=True, + substrs=["bitfield range -1:0 is not valid (negative index)"], + ) + self.expect( + "frame var 'value[0:-1]'", + error=True, + substrs=["bitfield range 0:-1 is not valid (negative index)"], + ) + + # A bitfield wider than 64 bits must be rejected. The underlying + # DataExtractor::GetMaxU64Bitfield only supports up to 64 bits + # (it asserts and otherwise performs an out-of-bounds shift). + self.expect( + "frame var 'value[0:64]'", + error=True, + substrs=["bitfield range 0:64 is not valid (more than 64 bits)"], + ) + + # A bitfield whose high index is past the base object's storage must be + # rejected. Otherwise reading/formatting the synthetic child performs an + # out-of-bounds shift in DataExtractor::GetMaxU64Bitfield. 'value' is a + # 32-bit int, so bit index 50 is out of range. The range is normalized + # (high:low swapped) before the message is built. + self.expect( + "frame var 'value[100:50]'", + error=True, + substrs=["bitfield range 50:100 is not valid"], + ) `````````` </details> https://github.com/llvm/llvm-project/pull/213055 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
