You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/10/11 16:06:03 UTC
[GitHub] [incubator-nuttx] fjpanag opened a new pull request, #7285: Fixed undefined behavior in UBSan.
fjpanag opened a new pull request, #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285
## Summary
Fixes an undefined behavior within UBSan.
Relevant issue #7274.
## Impact
UBSan is working correctly now.
## Testing
Tested with code that has an undefined behavior, and was triggering this code.
Before the patch UBSan was calling itself infinitely (eventually causing a crash), but after the change it behaves correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993274928
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
> Maybe it is better to keep code here as is and change is_inline_int to return bits < inline_bits; and not equal, so equal condition will be evaluated by return *(FAR int64_t *)val; in get_signed_val?
I don't see how this is going to help. In my case the `type_bit_width()` is 32 so the same code will be executed nevertheless.
But I don't understand, why do we need to clear the "extra_bits" in the first place?
Here are some real values:
Actual value is -1.
extra_bits = 32
val, ulong_val = 0xffffffffffffffff
So, this should return 0xffffffffffffffff as an int64_t, right?
Why clear any bits?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993562518
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
@pkarashchenko Your code seems to be correct. It yields correct results in all cases that I tested.
I just updated the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r995507247
##########
mm/ubsan/ubsan.c:
##########
@@ -158,9 +158,11 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
if (is_inline_int(type))
{
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
- uintptr_t ulong_val = (uintptr_t)val;
+ uint64_t mask = (1llu << extra_bits) - 1;
+ uint64_t ret = (uint64_t)val & mask;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(((ret & (1llu << (extra_bits - 1))) != 0) ?
+ ret | ~mask : ret);
Review Comment:
@pkarashchenko Originally I was using the Lua tests suite. Some tests (I guess purposely) were triggering UB's on the VM itself.
Then, after your comments, I also added the following:
```c
uint32_t a = -1;
uint16_t b = -1;
uint8_t c = -1;
uint32_t k = a << 2;
uint16_t l = b << 2;
uint8_t m = c << 2;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993876542
##########
mm/ubsan/ubsan.c:
##########
@@ -158,9 +158,11 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
if (is_inline_int(type))
{
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
- uintptr_t ulong_val = (uintptr_t)val;
+ uint64_t mask = (1llu << extra_bits) - 1;
+ uint64_t ret = (uint64_t)val & mask;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(((ret & (1llu << (extra_bits - 1))) != 0) ?
+ ret | ~mask : ret);
Review Comment:
I mean that:
1. 16 bits encoded value: `val` contain `0xFFFF` (`-1` in `int16_t`) and `type_bit_width(type)` is 16. Then `mask` will be `0xFFFF`, `ret` will be `0xFFFF`, `((ret & (1llu << (bits - 1))) != 0)` will be `0xFFFF & 0x8000 != 0` will be `true` and `0xFFFF | 0xFFFFFFFFFFFF0000` == `0xFFFFFFFFFFFFFFFF` (`-1` in `int64_t`) will be returned.
2. 32 bits encoded value: `val` contain `0xFFFFFFFF` (`-1` in `int32_t`) and `type_bit_width(type)` is 32. Then `mask` will be `0xFFFFFFFF`, `ret` will be `0xFFFFFFFF`, `((ret & (1llu << (bits - 1))) != 0)` will be `0xFFFFFFFF & 0x80000000 != 0` will be `true` and `0xFFFFFFFF | 0xFFFFFFFF00000000` == `0xFFFFFFFFFFFFFFFF` (`-1` in `int64_t`) will be returned.
3. 64 bits encoded value: `val` contain `0xFFFFFFFFFFFFFFFF` (`-1` in `int64_t`) and `type_bit_width(type)` is 64. Then `mask` will be `0xFFFFFFFFFFFFFFFF`, `ret` will be `0xFFFFFFFFFFFFFFFF`, `((ret & (1llu << (bits - 1))) != 0)` will be `0xFFFFFFFFFFFFFFFF & 0x8000000000000000 != 0` will be `true` and `0xFFFFFFFFFFFFFFFF | 0x0` == `0xFFFFFFFFFFFFFFFF` (`-1` in `int64_t`) will be returned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r994777994
##########
mm/ubsan/ubsan.c:
##########
@@ -158,9 +158,11 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
if (is_inline_int(type))
{
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
- uintptr_t ulong_val = (uintptr_t)val;
+ uint64_t mask = (1llu << extra_bits) - 1;
+ uint64_t ret = (uint64_t)val & mask;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(((ret & (1llu << (extra_bits - 1))) != 0) ?
+ ret | ~mask : ret);
Review Comment:
Maybe there is a way to test simulation in 32bit docker environment and see if that brings a difference
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993229457
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
> The signed vs unsigned shift has essential difference in populating a sign bit, so 0xffffffff << 1 >> 1 (for 32 bits) will be 0x7fffffff for unsigned and 0xffffffff for signed.
This is not true. Shifting negative numbers is undefined. This is why UBSan complains in the first place.
The change must be kept to ensure that only unsigned (or positive) numbers are shifted.
One solution that I can think of is to manually move the sign bit to the correct place.
Another solution would be to get rid of the shifts, and construct a mask to clear the bits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#issuecomment-1276523635
let's ignore the temp ci broken and merge this simple change directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993308230
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
https://docs.oracle.com/cd/E19120-01/open.solaris/816-5138/convert-2/index.html -- yeah, things can be even more tricky if signed extension is done implicitly. Let's try to avoid bit shift of value
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993441377
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
> I agree that right shift of negative values is undefined by C standard, but most compilers implement it to be compatible with integer division by 2
I agree that most systems behave in a similar fashion, but *technically* it is still UB.
It is not a horrible bug in itself, but as UBSan calls itself it causes infinite recursion, ending up in a system crash.
> Why are you saying that if value is -1 then ulong_val is 0xffffffffffffffff? Are you testing on 32 bit or 64 bit platform?
I mean that for 64 bit platform it should be true, but not for 32 bit platform
Yes I am running it on a 64-bit system, using the simulator.
Unfortunately, due to the overhead of UBSan and of the test code (that triggers this UB), I cannot run this on actual hardware.
I am having a look on your solution right now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993286949
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
I agree that right shift of negative values is undefined by C standard, but most compilers implement it to be compatible with integer division by 2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993301685
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
maybe we can make something like
```
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uint64_t mask = (1llu << extra_bits) - 1;
uint64_t ret = (uint64_t)val & mask;
return (int64_t)(ret & (1llu << (extra_bits - 1)) != 0 ? ret | ~mask : ret);
```
? Or something similar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r992642956
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
I'm a bit confused with this change. The signed vs unsigned shift has essential difference in populating a sign bit, so `0xffffffff << 1 >> 1` (for 32 bits) will be `0x7fffffff` for unsigned and `0xffffffff` for signed. What `get_signed_val` should return in case if `is_inline_int(type)` is evaluated to `true`? For 64 bits target it is obvious since `uintptr_t` has the same size as `int64_t` so `return (int64_t)(ulong_val << extra_bits >> extra_bits);` is equivalent to `*(FAR int64_t *)val`, but if the value is `int32_t` then this change will break the logic because sign bit will not be populated and `int32_t` `-1` will become `4294967295` `int64_t` that seems to be completely wrong.
Maybe it is better to keep code here as is and change `is_inline_int` to `return bits < inline_bits;` and not equal, so equal condition will be evaluated by `return *(FAR int64_t *)val;` in `get_signed_val`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993301685
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
maybe we can make something like
```
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uint64_t mask = 1llu << extra_bits;
uint64_t ret = (uint64_t)val & mask;
return (int64_t)(ret & (1ll << (extra_bits - 1)) != 0 ? ret | ~mask : ret);
```
? Or something similar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993301685
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
maybe we can make something like
```
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uint64_t mask = 1llu << extra_bits;
uint64_t ret = (uint64_t)val & mask;
return (int64_t)(ret & (1llu << (extra_bits - 1)) != 0 ? ret | ~mask : ret);
```
? Or something similar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993870810
##########
mm/ubsan/ubsan.c:
##########
@@ -158,9 +158,11 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
if (is_inline_int(type))
{
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
- uintptr_t ulong_val = (uintptr_t)val;
+ uint64_t mask = (1llu << extra_bits) - 1;
+ uint64_t ret = (uint64_t)val & mask;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(((ret & (1llu << (extra_bits - 1))) != 0) ?
+ ret | ~mask : ret);
Review Comment:
I re-read my code again and it is a bit faulty. I mean that it will work for 32 bit values, but other variants seems to be wrong. It should be:
```
unsigned bits = type_bit_width(type);
uint64_t mask = (1llu << bits) - 1;
uint64_t ret = (uint64_t)val & mask;
return (int64_t)(((ret & (1llu << (bits - 1))) != 0) ?
ret | ~mask : ret);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r994867932
##########
mm/ubsan/ubsan.c:
##########
@@ -158,9 +158,11 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
if (is_inline_int(type))
{
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
- uintptr_t ulong_val = (uintptr_t)val;
+ uint64_t mask = (1llu << extra_bits) - 1;
+ uint64_t ret = (uint64_t)val & mask;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(((ret & (1llu << (extra_bits - 1))) != 0) ?
+ ret | ~mask : ret);
Review Comment:
Could you please specify your test scenario? Maybe I can try to experiment at my end
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993290268
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
Why are you saying that if value is `-1` then `ulong_val` is `0xffffffffffffffff`? Are you testing on 32 bit or 64 bit platform?
I mean that for 64 bit platform it should be true, but not for 32 bit platform
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r993290268
##########
mm/ubsan/ubsan.c:
##########
@@ -160,7 +160,7 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
uintptr_t ulong_val = (uintptr_t)val;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(ulong_val << extra_bits >> extra_bits);
Review Comment:
Why are you saying that if value is `-1` then `ulong_val` is `0xffffffffffffffff`? Are you testing on 32 bit or 64 bit platform?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] fjpanag commented on a diff in pull request #7285: Fixed undefined behavior in UBSan.
Posted by GitBox <gi...@apache.org>.
fjpanag commented on code in PR #7285:
URL: https://github.com/apache/incubator-nuttx/pull/7285#discussion_r994365145
##########
mm/ubsan/ubsan.c:
##########
@@ -158,9 +158,11 @@ static int64_t get_signed_val(FAR struct type_descriptor *type,
if (is_inline_int(type))
{
unsigned extra_bits = sizeof(int64_t) * 8 - type_bit_width(type);
- uintptr_t ulong_val = (uintptr_t)val;
+ uint64_t mask = (1llu << extra_bits) - 1;
+ uint64_t ret = (uint64_t)val & mask;
- return ((int64_t)ulong_val) << extra_bits >> extra_bits;
+ return (int64_t)(((ret & (1llu << (extra_bits - 1))) != 0) ?
+ ret | ~mask : ret);
Review Comment:
@pkarashchenko I tried your new code, but in practice I don't see any difference. Both yield correct results.
As I see, no matter the original type of the value, it is always encoded as a 64-bit value.
For example, consider the following:
```
int16_t a = -1;
int16_t b = a << 2;
```
The above will trigger UBSan, but the variable `val` in function `get_signed_val()` will contain the value `0xFFFFFFFFFFFFFFFF`.
*(As said, I can only test on simulator / 64-bit)*
I trust you on this, and I will create a new PR with the suggested fix, but in practice both work, at least for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org