got rid of some implicit conversions to avoid UBSAN warnings#2990
got rid of some implicit conversions to avoid UBSAN warnings#2990firewave wants to merge 2 commits into
Conversation
8d03cd1 to
045d06f
Compare
|
|
||
| if (mSettings->force) | ||
| mSettings->maxConfigs = ~0U; | ||
| mSettings->maxConfigs = static_cast<int>(~0U); |
There was a problem hiding this comment.
that cast changes the sign. we should either have a large positive value or -1 explicitly. Probably a large positive value is wanted..
There was a problem hiding this comment.
This will be done implicitly anyways - I just doing it explicitly so UBSAN know it is actually intended.
There was a problem hiding this comment.
actually this was a "true positive" in my opinion so we should not just hide it. I'd rather either write a large positive value (fixed assignment) or -1 (to keep old behavior).. and I have the feeling that a large positive value would be preferable.
| std::istringstream istr(str); | ||
| istr >> ret; | ||
| return ret; | ||
| return static_cast<bigint>(ret); |
There was a problem hiding this comment.
It seems unfortunate to change the sign like this. I guess ret should be a bigint and if there is overflow ... I'm not sure if an exception would be preferable or how we should handle it..
There was a problem hiding this comment.
This will be done implicitly anyways - I just doing it explicitly so UBSAN know it is actually intended.
There was a problem hiding this comment.
I rather fix the warning properly.
| const std::string s = value1.tokvalue->strValue(); | ||
| const MathLib::bigint index = value2.intvalue; | ||
| if (index == s.size()) { | ||
| if (static_cast<std::string::size_type>(index) == s.size()) { |
There was a problem hiding this comment.
In my opinion it was a mistake to make std::string::size() return value unsigned. I'd rather use the ssize() that will be available in c++20.. maybe introduce a ssize() function in utils.h will work for now.
There was a problem hiding this comment.
This will be done implicitly anyways - I just doing it explicitly so UBSAN know it is actually intended.
There was a problem hiding this comment.
I know what you are saying but I dislike putting casts in the code. The code is changed constantly. Imagine that we someday change bigint so it's a int128_t. Now your cast could cause loss of precision. The s.size() should be promoted.
| const MathLib::biguint unsignedMaxValue = (1ULL << (sz * 8)) - 1ULL; | ||
| const MathLib::biguint signBit = 1ULL << (sz * 8 - 1); | ||
| value.intvalue &= unsignedMaxValue; | ||
| value.intvalue &= static_cast<long long>(unsignedMaxValue); |
There was a problem hiding this comment.
I do not see a safety improvement here.. just a cast. I question that it improves the readability. And it is dangerous to use casts it hides sanitizer/compiler warnings ...
There was a problem hiding this comment.
This will be done implicitly anyways - I just doing it explicitly so UBSAN know it is actually intended.
There was a problem hiding this comment.
about conversion warnings.. if there are bugs I rather fix the bugs. If there are not bugs I do not want to add casts because in the long run those could hide real bugs. I think that rewriting the code can be an option sometimes, unsigned types should be avoided.
I guess that in this case the unsignedMaxValue does not need to be unsigned. We know that the value of sz is 1-7.
|
|
||
| // Enum scope and type | ||
| ASSERT_EQUALS(3, db->scopeList.size()); | ||
| ASSERT_EQUALS(3UL, db->scopeList.size()); |
There was a problem hiding this comment.
The tool is too stupid here in my opinion. The original code looks fine as far as I see.. what possible real danger could there be.
There was a problem hiding this comment.
This is just so the assertEquals() can be replaced with a template later on. That requires the same types on both sides. The code is completely inconsistent and this just offers some consistency. This doesn't fix any UBSAN warnings yet. Those only happen with long long and unsigned long long are involved and are still present.
There was a problem hiding this comment.
Imho I think the original code is nicer. Template limitations shouldn't be worked around in our test code.
|
seems ubsan is just as noisy as compilers. A tool that only writes genuine warnings would be interesting. |
Extracted from #2922 since that needs more work regarding older Visual Studio versions.
There's probably some compiler warnings which would point these out at compile-time. But that's something for a later date.