Details
-
Bug
-
Status: Closed (View Workflow)
-
Major
-
Resolution: Fixed
-
10.2(EOL), 10.3(EOL), 10.4(EOL), 10.5
Description
As noted in MDEV-20377, inline assembler will cause MemorySanitizer to report false positives for uninitialized values. It would be more portable to use the compiler built-in functions not only on Windows, but on all platforms.
Here is a crude patch for WolfSSL:
diff --git a/wolfcrypt/src/random.c b/wolfcrypt/src/random.c
|
index 6b0d5dafc..817619d16 100644
|
--- a/wolfcrypt/src/random.c
|
+++ b/wolfcrypt/src/random.c
|
@@ -173,7 +173,7 @@ int wc_RNG_GenerateByte(WC_RNG* rng, byte* b)
|
static int wc_GenerateRand_IntelRD(OS_Seed* os, byte* output, word32 sz);
|
#endif
|
|
-#ifdef USE_WINDOWS_API
|
+#if 1 /*def USE_WINDOWS_API */
|
#include <immintrin.h>
|
#endif /* USE_WINDOWS_API */
|
#endif
|
@@ -1282,7 +1282,7 @@ int wc_FreeNetRandom(void)
|
|
#ifdef HAVE_INTEL_RDSEED
|
|
-#ifndef USE_WINDOWS_API
|
+#if 0/*ndef USE_WINDOWS_API*/
|
|
/* return 0 on success */
|
static WC_INLINE int IntelRDseed64(word64* seed)
|
@@ -1302,7 +1302,7 @@ int wc_FreeNetRandom(void)
|
{
|
int ok;
|
|
- ok = _rdseed64_step(seed);
|
+ ok = _rdseed64_step((unsigned long long*) seed);
|
return (ok) ? 0 : -1;
|
}
|
|
|
and another for InnoDB:
diff --git a/storage/innobase/ut/ut0crc32.cc b/storage/innobase/ut/ut0crc32.cc
|
index 4a6447c1dcf..58273e9058e 100644
|
--- a/storage/innobase/ut/ut0crc32.cc
|
+++ b/storage/innobase/ut/ut0crc32.cc
|
@@ -219,6 +219,8 @@ ut_crc32_8_hw(
|
{
|
#ifdef _MSC_VER
|
*crc = _mm_crc32_u8(*crc, (*data)[0]);
|
+#elif 1
|
+ *crc = __builtin_ia32_crc32qi(*crc, (*data)[0]);
|
#else
|
asm("crc32b %1, %0"
|
/* output operands */
|
@@ -251,6 +253,8 @@ ut_crc32_64_low_hw(
|
#else
|
#error Not Supported processors type.
|
#endif
|
+#elif 1
|
+ crc_64bit = __builtin_ia32_crc32di(crc_64bit, data);
|
#else
|
asm("crc32q %1, %0"
|
/* output operands */ |
Both these patches will require some compiler switches to enable the instructions to be emitted. For clang 8.0.1, -march=native did the trick for me, but that is obviously unacceptable. According to diagnostic messages from the compiler, the features might be called "rdseed" and "SSE 4.2".
Attachments
Issue Links
- is part of
-
-
- Closed
-
- relates to
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
- links to
Activity
Very well, I can file a separate bug against WolfSSL. Meanwhile, can you please look at the InnoDB part?
This is blocking the use of MSAN, which is an important tool for quality assurance. It promises to have only 3× CPU overhead, which is much lower than Valgrind. Hence, "won’t fix" is not an acceptable resolution.
Replace inline asm with compiler built-in intrinsic functions #2415 was filed against WolfSSL.
I'm closing this since this is not in our code, and since the bug was already files against WolfSSL.
I am reopening this, because MDEV-20377 (enable MemorySanitizer) requires that the problem be addressed.
The WolfSSL problem was worked around by disabling all acceleration in WITH_MSAN=ON builds.
The InnoDB CRC-32C implementation now uses Intel intrinsic functions. Only in GCC 4 we will have to use __builtin_ functions, because the header <nmmintrin.h> is not compatible with that compiler.
I submitted WolfSSL pull request #3268 to use the Intel intrinsic functions for RDRAND and RDSEED whenever applicable and made a test build.
My pull request was merged to WolfSSL, and we can actually enable the rdrand and rdseed instructions in WITH_MSAN builds. But, we still must disable the hand-written assembler code until some instrumentation is added to WolfSSL. That should involve invoking __msan_check_mem_is_initialized() on the input and __msan_unpoison() on the output of the assembler code.
note: -march flags/test still required (10.3-d99f787244ab82f658b3f4a6c9877289e6385e04
|
clang++-10.0.1 |
[719/1511] Building CXX object storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o
|
FAILED: storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o
|
ccache /usr/bin/clang++ -DBTR_CUR_ADAPT -DBTR_CUR_HASH_ADAPT -DCOMPILER_HINTS -DDBUG_TRACE -DHAVE_CONFIG_H -DHAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE=1 -DHAVE_IB_LINUX_FUTEX=1 -DHAVE_LZ4=1 -DHAVE_LZ4_COMPRESS_DEFAULT=1 -DHAVE_LZMA=1 -DHAVE_NANOSLEEP=1 -DHAVE_SCHED_GETCPU=1 -DHAVE_SNAPPY=1 -DLINUX_NATIVE_AIO=1 -DMUTEX_EVENT -DWITH_INNODB_DISALLOW_WRITES -D_FILE_OFFSET_BITS=64 -Iinclude -I/home/dan/repos/mariadb-server-10.3/storage/innobase/include -I/home/dan/repos/mariadb-server-10.3/storage/innobase/handler -I/home/dan/repos/mariadb-server-10.3/libbinlogevents/include -I/home/dan/repos/mariadb-server-10.3/include -I/home/dan/repos/mariadb-server-10.3/sql -fstack-protector --param=ssp-buffer-size=4 -fno-rtti -O2 -g -DNDEBUG -fsanitize=memory -fsanitize-memory-track-origins -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DDBUG_OFF -Wall -Wdeclaration-after-statement -Wextra -Wformat-security -Wno-init-self -Wno-null-conversion -Wno-unused-parameter -Wno-unused-private-field -Woverloaded-virtual -Wnon-virtual-dtor -Wvla -Wwrite-strings -DUNIV_LINUX -D_GNU_SOURCE=1 -fvisibility=hidden -MD -MT storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o -MF storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o.d -o storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o -c /home/dan/repos/mariadb-server-10.3/storage/innobase/ut/ut0crc32.cc
|
/home/dan/repos/mariadb-server-10.3/storage/innobase/ut/ut0crc32.cc:202:9: error: '__builtin_ia32_crc32qi' needs target feature sse4.2
|
*crc = __builtin_ia32_crc32qi(*crc, (*data)[0]);
|
^
|
/home/dan/repos/mariadb-server-10.3/storage/innobase/ut/ut0crc32.cc:236:14: error: '__builtin_ia32_crc32di' needs target feature sse4.2
|
crc_64bit = __builtin_ia32_crc32di(crc_64bit, data);
|
^
|
2 errors generated.
|
danblack, I did not claim to fix earlier versions than 10.5. The main motivation of this is to be able to use WITH_MSAN without any -march. There have been a lot of fixes in 10.5 to the CRC-32 and CRC-32C code that we did not port to earlier versions. Does the 10.3 build fail on a platform where clang is the default compiler, such as FreeBSD or Mac OS X?
pushed https://travis-ci.org/github/grooverdan/mariadb-server/builds/738179878 as OSX test with MSAN enabled.
danblack, I updated the bb-10.5-MDEV-20386 branch again. I am anticipating a WolfSSL release soon. (In our main branches, we use release tags for the submodule, not ‘random’ commits.)
MDEV-24514 will enable the hand-written assembler code (WOLFSSL_AESNI) when MemorySanitizer is enabled.
You may file a bug against wolfssl/wolfcrypt. we do not change WolfSSL code