herumi / mcl

a portable and fast pairing-based cryptography library
BSD 3-Clause "New" or "Revised" License
450 stars 151 forks source link

Undefined behavior (signed integer overflow) #187

Closed guidovranken closed 10 months ago

guidovranken commented 10 months ago
export CC=clang
export CXX=clang++
export CFLAGS="-fsanitize=undefined -g"
export CXXFLAGS="-fsanitize=undefined -g"
git clone --depth 1 https://github.com/herumi/mcl.git
cd mcl/
make bint_header
mkdir build/
cd build/
cmake .. -DMCL_STATIC_LIB=on
make -j$(nproc)

Then compile and run the following program:

#include <mcl/bls12_381.hpp>

int main(void)
{
    mcl::bn::initPairing(::mcl::BLS12_381);
    return 0;
}
$CXX $CXXFLAGS -I mcl/include/ p.cpp mcl/build/lib/libmcl.a

Output:

/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:153:14: runtime error: signed integer overflow: -3608227726454314319 * 7 cannot be represented in type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:153:14 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:137:5: runtime error: left shift of negative value -1
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:137:5 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:138:5: runtime error: left shift of negative value -1
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:138:5 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:209:11: runtime error: signed integer overflow: 3894487790653734915 * -252201579132747776 cannot be represented in type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:209:11 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:210:11: runtime error: signed integer overflow: 3894487790653734915 * 3831622457289030346 cannot be represented in type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:210:11 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:208:22: runtime error: signed integer overflow: -8260219669427199326 + -5044313057631688021 cannot be represented in type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:208:22 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:207:32: runtime error: signed integer overflow: -5044313057631688021 * 17592186044416 cannot be represented in type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:207:32 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:208:32: runtime error: signed integer overflow: -5044313057631688021 * 3025100429 cannot be represented in type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:208:32 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:207:22: runtime error: signed integer overflow: -7495706186083699216 + -6340040029238385324 cannot be represented in type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:207:22 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:154:10: runtime error: signed integer overflow: 5 * 3314367850767908865 cannot be represented in type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:154:10 in 
/mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:154:5: runtime error: signed integer overflow: -4611686018427305549 + -4611686018427616755 cannot be represented in type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /mnt/2tb/mcl-ubsan/mcl/include/mcl/invmod.hpp:154:5 in 

This was introduced very recently.

herumi commented 10 months ago

Thank you for the report. I already know that. It is due to too many undefined operations of signed integer operations in C++. For example, (-1) >> 3 is undefined behavior (UB) before C++20 I want an arithmetic right shift, sar eax, 3. The written code to avoid UB will become dirty and slow. I check the current code runs well on x64/M1(AArch64)/Wasm, so please wait until I find a better solution.

herumi commented 10 months ago

I could avoid UB by minimizing signed operations.

guidovranken commented 10 months ago

I think avoiding the UB is better.

I check the current code runs well on x64/M1(AArch64)/Wasm, so please wait until I find a better solution.

It can work well now, but suddenly break if another compiler or optimization level is used.

herumi commented 10 months ago

Okay, I'll add the UB tests to GiHub Action.

As I wrote in one previous message, the UB is not reported in the latest version.

herumi commented 10 months ago

The latest version runs sanitize=undefined on GitHub Action. https://github.com/herumi/mcl/commit/7336003cc691ba5b94fce66c272e1fc085a57116