llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Yao Qi (qiyao)
<details>
<summary>Changes</summary>
The DIL assignment path (`frame variable 'lhs = rhs'`) routes through
`ValueObject::SetValueFromInteger`, which verifies that the new value fits
in the destination type before writing it. That size check compared the
source `APInt`'s raw 64-bit word, treated as an unsigned value, against the
destination type's unsigned maximum:
```
uint64_t u_max = (1 << (byte_size * CHAR_BIT)) - 1;
if (*(value.getRawData()) > u_max)
return llvm::createStringError("Illegal argument: new value is too big");
```
This is wrong in both directions:
* Small negative values that fit are rejected. A literal such as `-2` has a
32-bit `APInt` of `0xFFFFFFFE`, whose raw word is `0xFFFFFFFFFFFFFFFE`.
Compared as unsigned against a short's `0xFFFF` it looks "too big", so
```
(lldb) frame variable 's = -2'
error: Illegal argument: new value is too big
```
even though -2 fits a `short`.
* Positive values that overflow the signed range are accepted and silently
wrap and stored a value that displayed as `-32768`.
```
(lldb) frame variable 's = 32768'
(short) s = -32768
```
Check the number of significant bits with respect to the destination's
signedness instead: `getSignificantBits()` for a signed destination and
`getActiveBits()` for an unsigned one. This accepts the valid range
(`[-32768, 32767]` for a signed short) and rejects everything outside it.
Also extend the value before building the `DataExtractor` used to write it.
The extractor reads exactly `byte_size` bytes from the `APInt`'s raw storage,
so a value narrower than the destination must be extended to cover the full
read. Use a sign extension so negative values keep their value in the wider
destination.
Extends the DIL assignment API test (`TestFrameVarDILAssign`) to cover
assigning negative and boundary values to the existing `short` variable:
`-2`, `-4` (from `int j`), and the `[-32768, 32767]` boundaries are accepted,
while `32768` and `-32769` are rejected. Without the fix these narrow-type
assignments either fail with "new value is too big" or silently wrap.
---
Full diff: https://github.com/llvm/llvm-project/pull/207404.diff
2 Files Affected:
- (modified) lldb/source/ValueObject/ValueObject.cpp (+18-9)
- (modified)
lldb/test/API/commands/frame/var-dil/expr/Assignment/TestFrameVarDILAssign.py
(+24)
``````````diff
diff --git a/lldb/source/ValueObject/ValueObject.cpp
b/lldb/source/ValueObject/ValueObject.cpp
index 32332c2962e87..ea575a57e6523 100644
--- a/lldb/source/ValueObject/ValueObject.cpp
+++ b/lldb/source/ValueObject/ValueObject.cpp
@@ -1255,18 +1255,27 @@ llvm::Error ValueObject::SetValueFromInteger(const
llvm::APInt &value,
// Exclude size check when assigning an integer 1 or 0 to a boolean.
if (!val_type.IsBoolean() || (!value.isOne() && !value.isZero())) {
byte_size = llvm::expectedToOptional(GetByteSize()).value_or(0);
- if (value.getBitWidth() > byte_size * CHAR_BIT) {
- // The type is too big, but maybe the value itself is small enough?
- uint64_t u_max = (1 << (byte_size * CHAR_BIT)) - 1;
- if (*(value.getRawData()) > u_max)
- return llvm::createStringError(
- "Illegal argument: new value is too big");
- }
- }
+ // Check that the value is representable in the destination type.
+ unsigned dest_bits = byte_size * CHAR_BIT;
+ unsigned needed_bits = val_type.IsSigned() ? value.getSignificantBits()
+ : value.getActiveBits();
+ if (needed_bits > dest_bits)
+ return llvm::createStringError("Illegal argument: new value is too big");
+ }
+
+ // The DataExtractor below reads exactly byte_size bytes from the APInt's raw
+ // storage. If the incoming value has fewer bits than the destination type,
+ // reading byte_size bytes could run past the APInt's backing store and pull
+ // in garbage (an out-of-bounds read). Extend the value so its storage always
+ // covers the full read, preserving the sign so that negative values keep
+ // their value in the wider destination.
+ llvm::APInt sized_value = value;
+ if (sized_value.getBitWidth() < byte_size * CHAR_BIT)
+ sized_value = sized_value.sext(byte_size * CHAR_BIT);
Status error;
lldb::DataExtractorSP data_sp = std::make_shared<DataExtractor>(
- reinterpret_cast<const void *>(value.getRawData()), byte_size,
+ reinterpret_cast<const void *>(sized_value.getRawData()), byte_size,
target->GetArchitecture().GetByteOrder(),
static_cast<uint8_t>(target->GetArchitecture().GetAddressByteSize()));
SetData(*data_sp, error);
diff --git
a/lldb/test/API/commands/frame/var-dil/expr/Assignment/TestFrameVarDILAssign.py
b/lldb/test/API/commands/frame/var-dil/expr/Assignment/TestFrameVarDILAssign.py
index 5b0ba3e8e8a72..5734395cf0ff0 100644
---
a/lldb/test/API/commands/frame/var-dil/expr/Assignment/TestFrameVarDILAssign.py
+++
b/lldb/test/API/commands/frame/var-dil/expr/Assignment/TestFrameVarDILAssign.py
@@ -182,3 +182,27 @@ def test_assignment(self):
error=True,
substrs=["new value is too big"],
)
+
+ # A negative value that is representable in the (signed) destination
+ # type must be accepted.
+ self.expect("frame variable 's = -2'", substrs=["s = -2"])
+ self.expect_var_path("s", value="-2")
+ self.expect("frame variable 's = j'", substrs=["s = -4"])
+ self.expect_var_path("s", value="-4")
+ # The boundary values of a signed short must be accepted.
+ self.expect("frame variable 's = 32767'", substrs=["s = 32767"])
+ self.expect_var_path("s", value="32767")
+ self.expect("frame variable 's = -32768'", substrs=["s = -32768"])
+ self.expect_var_path("s", value="-32768")
+ # A positive value that overflows the signed range must be rejected
+ # instead of silently wrapping.
+ self.expect(
+ "frame variable 's = 32768'",
+ error=True,
+ substrs=["new value is too big"],
+ )
+ self.expect(
+ "frame variable 's = -32769'",
+ error=True,
+ substrs=["new value is too big"],
+ )
``````````
</details>
https://github.com/llvm/llvm-project/pull/207404
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
