Undefined behavior shift in CompileHelper.cpp

[hzeller]

Testing with undefined behavior sanitizer finds that in CompileHelper.cpp:4044 an invalid shift is used. Shifting outside the width of an integer is UB

This can be seen with ArianeElab, ArianeElab2, HierPathOverride and LargeHexCast

src/DesignCompile/CompileHelper.cpp:4044:42: runtime error: shift exponent 1023 is too large for 32-bit type 'int'
    #0 0x55e69b64f7ed in SURELOG::CompileHelper::expandPatternAssignment(UHDM::typespec const*, UHDM::expr*, SURELOG::DesignComponent*, SURELOG::CompileDesign*, SURELOG::ValuedComponentI*) src/DesignCompile/CompileHelper.cpp:4044:42
    #1 0x55e69b757342 in SURELOG::NetlistElaboration::elab_parameters_(SURELOG::ModuleInstance*, bool) src/DesignCompile/NetlistElaboration.cpp:228:32
    #2 0x55e69b755c3b in SURELOG::NetlistElaboration::elaborate_(SURELOG::ModuleInstance*, bool) src/DesignCompile/NetlistElaboration.cpp:497:3
    #3 0x55e69b7558ba in SURELOG::NetlistElaboration::elaborateInstance(SURELOG::ModuleInstance*) src/DesignCompile/NetlistElaboration.cpp:112:10
    #4 0x55e69b79224e in SURELOG::DesignElaboration::elaborateInstance_(SURELOG::FileContent const*, SURELOG::NodeId, SURELOG::NodeId, SURELOG::ModuleInstanceFactory*, SURELOG::ModuleInstance*, SURELOG::Config*, std::__u::vector<SURELOG::ModuleInstance*, std::__u::allocator<SURELOG::ModuleInstance*>>&) src/DesignCompile/DesignElaboration.cpp:896:12
    #5 0x55e69b799d56 in SURELOG::DesignElaboration::elaborateInstance_(SURELOG::FileContent const*, SURELOG::NodeId, SURELOG::NodeId, SURELOG::ModuleInstanceFactory*, SURELOG::ModuleInstance*, SURELOG::Config*, std::__u::vector<SURELOG::ModuleInstance*, std::__u::allocator<SURELOG::ModuleInstance*>>&) src/DesignCompile/DesignElaboration.cpp:1707:15
    #6 0x55e69b78bd9d in SURELOG::DesignElaboration::elaborateModule_(std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>> const&, SURELOG::FileContent const*, bool) src/DesignCompile/DesignElaboration.cpp:549:11
    #7 0x55e69b77ecae in elaborateAllModules_ src/DesignCompile/DesignElaboration.cpp:477:10
    #8 0x55e69b77ecae in SURELOG::DesignElaboration::elaborate() src/DesignCompile/DesignElaboration.cpp:76:3
    #9 0x55e69b721897 in SURELOG::CompileDesign::elaboration_() src/DesignCompile/CompileDesign.cpp:368:13
    #10 0x55e69b72166f in SURELOG::CompileDesign::elaborate() src/DesignCompile/CompileDesign.cpp:272:11
    #11 0x55e69b59e7f0 in SURELOG::Compiler::compile() src/SourceCompile/Compiler.cpp:1047:24
    #12 0x55e69b500f54 in SURELOG::start_compiler(SURELOG::CommandLineParser*) src/API/Surelog.cpp:33:31
    #13 0x55e69b4faa32 in executeCompilation(int, char const**, bool, bool, SURELOG::ErrorContainer::Stats*) src/main.cpp:92:36
    #14 0x55e69b4fd73c in main src/main.cpp:323:21

Besides fixing this: it would probably make sense to set up asan, msan and ubsan tests in the CI.

[alaindargelas]

I can't reproduce any of these errors with GCC. I tried to set a comparison of the value to 32, it never hits in the entire regression.
I can't manage install clang on my machine either due to this error:

sudo apt-get install clang
[sudo] password for alain:
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
Some packages could not be installed. This may mean that you have
requested an impossible situation or if you are using the unstable
distribution that some required packages have not yet been created
or been moved out of Incoming.
The following information may help to resolve the situation:

The following packages have unmet dependencies:
libobjc-9-dev : Depends: gcc-9-base (= 9.4.0-1ubuntu120.04.1) but 9.4.0-3ubuntu1.1 is to be installed
Depends: libgcc-9-dev (= 9.4.0-1ubuntu120.04.1) but 9.4.0-3ubuntu1.1 is to be installed
E: Unable to correct problems, you have held broken packages.

[alaindargelas]

Is this a real error?

[hzeller]

If you shift a 32 bit number by 1023 bits, it sounds like a real error.
In particular, it is undefined behavior: even if it is meant to 'just shift out until there are only zeroes', this is not what will happen.

[hzeller]

The code in question is something like

values[valIndex] = (v & (1ULL << (csize - 1 - i))) ? 1 : 0;

so is csize some very large value ? then the code might not do what one expect it to do.

[alaindargelas]

I can't find any testcase that has that kind of shift.
No tests in Surelog regression exhibit this behavior with the GCC compiler. I instrumented the code, I never reach this illegal condition.

[alaindargelas]

Can you reproduce the error with GCC, if yes, please tell me which test and eventual code change to create an assert.

[hzeller]

With the recent changes, this location now moved to
src/DesignCompile/CompileHelper.cpp:4617

In my address sanitizer set-up I get the some tests that create an issue, e.g.:

• HierPathOverride (shift exponent 71)
• LargeHexCast (shift exponent 95)
• ArianeElab (shift exponent 1023)
• ArianeElab2 (shift exponent 1023)

The error message from asan is something like

 src/DesignCompile/CompileHelper.cpp:4617:45: runtime error: shift exponent 71 is too large for 64-bit type 'unsigned long long'

You can instrument is by adding something like this:

--- a/src/DesignCompile/CompileHelper.cpp
+++ b/src/DesignCompile/CompileHelper.cpp
@@ -4614,6 +4614,13 @@ UHDM::expr* CompileHelper::expandPatternAssignment(const typespec* tps,
               if (valIndex > (int32_t)(size - 1)) {
                 break;
               }
+              {
+                int shift = (csize - 1 - i);
+                if (shift < 0 || shift >= 64) {
+                  fprintf(stderr, "%s: shifting by %d\n", __FILE__, shift);
+                  abort();
+                }
+              }
               values[valIndex] = (v & (1ULL << (csize - 1 - i))) ? 1 : 0;
               valIndex++;
             }

[alaindargelas]

@hzeller, @hs-apotell , can you tell me how you compile Surelog with clang?
I tried
export CXX clang-12
export CC clang-12
make

I get the GNU linker and it errors out (rightfully)
then I added the following to try to force the llvm linker:
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fuse-ld=/usr/bin/llvm-link-12")
I'm getting linker options errors...

[hs-apotell]

I haven't tried with v12. I have v13 installed. It might be a version issue.
Also, I use WSL on Windows.

export CC=clang
export CXX=clang++
make

[alaindargelas]

Thanks! I think I also got my settings incorrect, it should be:
export CXX=clang++-12
export CC=clang-12

[alaindargelas]

A first order fix is done in #3641.