got rid of some implicit conversions to avoid UBSAN warnings #2990

firewave · 2020-12-27T13:01:05Z

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.

danmar · 2020-12-27T15:13:21Z

cli/cmdlineparser.cpp

@@ -925,7 +925,7 @@ bool CmdLineParser::parseFromArgs(int argc, const char* const argv[])
        mSettings->checkAllConfigurations = true;

    if (mSettings->force)
-        mSettings->maxConfigs = ~0U;
+        mSettings->maxConfigs = static_cast<int>(~0U);


that cast changes the sign. we should either have a large positive value or -1 explicitly. Probably a large positive value is wanted..

This will be done implicitly anyways - I just doing it explicitly so UBSAN know it is actually intended.

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.

I changed this one with e7c462b

danmar · 2020-12-27T15:16:22Z

lib/mathlib.cpp

@@ -549,7 +549,7 @@ MathLib::bigint MathLib::toLongNumber(const std::string & str)
    biguint ret = 0;
    std::istringstream istr(str);
    istr >> ret;
-    return ret;
+    return static_cast<bigint>(ret);


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..

This will be done implicitly anyways - I just doing it explicitly so UBSAN know it is actually intended.

I rather fix the warning properly.

danmar · 2020-12-27T15:21:53Z

lib/valueflow.cpp

@@ -792,7 +792,7 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti
                    if (value1.tokvalue->tokType() == Token::eString) {
                        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()) {


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.

This will be done implicitly anyways - I just doing it explicitly so UBSAN know it is actually intended.

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.

danmar · 2020-12-27T15:25:25Z

lib/valueflow.cpp

@@ -3997,9 +3997,9 @@ static std::list<ValueFlow::Value> truncateValues(std::list<ValueFlow::Value> va
        if (value.isIntValue() && sz > 0 && sz < 8) {
            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);


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 ...

This will be done implicitly anyways - I just doing it explicitly so UBSAN know it is actually intended.

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.

danmar · 2020-12-27T15:26:49Z

test/testclangimport.cpp

@@ -1015,7 +1015,7 @@ class TestClangImport: public TestFixture {
        GET_SYMBOL_DB(clang);

        // Enum scope and type
-        ASSERT_EQUALS(3, db->scopeList.size());
+        ASSERT_EQUALS(3UL, db->scopeList.size());


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.

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.

Imho I think the original code is nicer. Template limitations shouldn't be worked around in our test code.

danmar · 2021-05-05T05:43:50Z

seems ubsan is just as noisy as compilers. A tool that only writes genuine warnings would be interesting.

firewave added 2 commits December 27, 2020 14:56

adjusted ASSERT_EQUALS() checks so types are the same on both sides

f1d7d0f

made some conversions explicit to avoid UBSAN warnings

045d06f

firewave force-pushed the test-assert branch from 8d03cd1 to 045d06f Compare December 27, 2020 13:57

danmar reviewed Dec 27, 2020

View reviewed changes

firewave marked this pull request as draft January 18, 2021 16:41

danmar closed this May 5, 2021

got rid of some implicit conversions to avoid UBSAN warnings #2990

got rid of some implicit conversions to avoid UBSAN warnings #2990

Uh oh!

Conversation

firewave commented Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar commented May 5, 2021

Uh oh!

Uh oh!

firewave commented Dec 27, 2020 •

edited

Loading

danmar Dec 27, 2020 •

edited

Loading

danmar Dec 27, 2020 •

edited

Loading