The strncpy() wrapper that was introduced in
commit 567b681299
is checking whether the output was truncated even in cases
where the caller does not care about it.
Let us introduce a separate function safe_strcpy_truncated() that
indidates whether the output was truncated.
BASE 62 uses 0-9, A-Z and then a-z to give the numbers 0-61. This patch
increases the range of the string functions to cover this.
Based on ideas and tests in PR #2589, but re-written into the charset
functions.
Includes fix by Sergei, UBSAN complained:
ctype-simple.c:683:38: runtime error: negation of -9223372036854775808
cannot be represented in type 'long long int'; cast to an unsigned
type to negate this value to itself
Co-authored-by: Weijun Huang <huangweijun1001@gmail.com>
Co-authored-by: Sergei Golubchik <serg@mariadb.org>
The `safe_strcpy()` function was added in
https://github.com/mariadb/server/commit/567b68129943#diff-23f88d0b52735bf79b7eb76e2ddbbebc96f3b1ca16e784a347525a9c43134d77
Unfortunately, its current implementation triggers many GCC 8+ string
truncation and array bounds warnings, particularly due to the potential
for a false positive `-Warray-bounds`.
For example, the line `safe_strcpy(delimiter, sizeof(delimiter), ";")` in
`client/mysqldump.c` causes the following warning:
[1669/1914] Building C object client/CMakeFiles/mariadb-dump.dir/mysqldump.c.o
In file included from /PATH/include/my_sys.h:20,
from /PATH/mysqldump.c:51:
In function ?safe_strcpy?,
inlined from ?dump_events_for_db.isra? at /PATH/client/mysqldump.c:2595:3:
/PATH/include/m_string.h:258:39: warning: array subscript 1535 is outside array bounds of ?const char[2]? [-Warray-bounds=]
258 | if (dst[dst_size - 2] != '\0' && src[dst_size - 1] != '\0')
| ~~~^~~~~~~~~~~~~~
GCC is reporting that the `safe_strcpy` function *could* cause an
out-of-bounds read from the constant *source* string `";"`, however this
warning is unhelpful and confusing because it can only happen if the size of
the *destination* buffer is incorrectly specified, which is not the case
here.
In https://github.com/MariaDB/server/pull/2640, Andrew Hutchings proposed
fixing this by disabling the `-Warray-bounds` check in this function
(specifically in
be382d01d0 (diff-23f88d0b52735bf79b7eb76e2ddbbebc96f3b1ca16e784a347525a9c43134d77R255-R262)).
However, this was rejected because it also disables the *helpful*
`-Warray-bounds` check on the destination buffer.
Cherry-picking the commit
a7adfd4c52
from 11.2 by Monty Widenius solves the first two problems:
1. It reimplements `safe_strcpy` a bit more efficiently, skipping the
`memset(dst, 0, dst_size)`. This is unnecessary since `strncpy` already
pads `dst` with 0 bytes.
2. It will not trigger the `-Warray-bounds` warning, because `src` is
not read based on an offset determined from `dst_size`.
There is a third problem, however. Using `strncpy` triggers the
`-Wstringop-truncation` warning
(https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-truncation),
so we need to disable that. However, that is a much less broadly and
generally-useful warning so there is no loss of static analysis value caused
by disabling it.
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the
BSD-new license. I am contributing on behalf of my employer Amazon Web
Services, Inc.
This is allowed:
STRING_WITH_LEN("string literal")
This is not:
char *str = "pointer to string";
... STRING_WITH_LEN(str) ..
In C++ this is also allowed:
const char str[] = "string literal";
... STRING_WITH_LEN(str) ...
The MariaDB code base uses strcat() and strcpy() in several
places. These are known to have memory safety issues and their usage is
discouraged. Common security scanners like Flawfinder flags them. In MariaDB we
should start using modern and safer variants on these functions.
This is similar to memory issues fixes in 19af1890b5
and 9de9f105b5 but now replace use of strcat()
and strcpy() with safer options strncat() and strncpy().
However, add '\0' forcefully to make sure the result string is correct since
for these two functions it is not guaranteed what new string will be null-terminated.
Example:
size_t dest_len = sizeof(g->Message);
strncpy(g->Message, "Null json tree", dest_len); strncat(g->Message, ":",
sizeof(g->Message) - strlen(g->Message)); size_t wrote_sz = strlen(g->Message);
size_t cur_len = wrote_sz >= dest_len ? dest_len - 1 : wrote_sz;
g->Message[cur_len] = '\0';
All new code of the whole pull request, including one or several files
that are either new files or modified ones, are contributed under the BSD-new
license. I am contributing on behalf of my employer Amazon Web Services
-- Reviewer and co-author Vicențiu Ciorbaru <vicentiu@mariadb.org>
-- Reviewer additions:
* The initial function implementation was flawed. Replaced with a simpler
and also correct version.
* Simplified code by making use of snprintf instead of chaining strcat.
* Simplified code by removing dynamic string construction in the first
place and using static strings if possible. See connect storage engine
changes.
I found that memcpy_aligned was used incorrectly at redo log and decided to put
assertions in aligned functions. And found even more incorrect cases.
Given the amount discovered of bugs, I left assertions to prevent future bugs.
my_assume_aligned(): instead of MY_ASSUME_ALIGNED macro
Introduce memcpy_aligned<N>(), memcmp_aligned<N>(), memset_aligned<N>()
and use them for accessing InnoDB page header fields that are known
to be aligned.
MY_ASSUME_ALIGNED(): Wrapper for the GCC/clang __builtin_assume_aligned().
Nothing similar seems to exist in Microsoft Visual Studio, and the
C++20 std::assume_aligned is not available to us yet.
Explicitly specified alignment guarantees allow compilers to generate
faster code on platforms with strict alignment rules, instead of
emitting calls to potentially unaligned memcpy(), memcmp(), or memset().
strmake_buf() macro should only be used with char[] arrays,
and never with char* pointers. To distinguish between the two
we create a new variable of the same type and initialize it
using array initializer, this causes compilation failure
with pointers. The variable is unused and will be removed by the
compiler. It's enough to do this check only with gcc, so
it doesn't have to be portable.
find_type_or_exit() client helper did exit(1) on error, exit(1) moved to
clients.
mysql_read_default_options() did exit(1) on error, error is passed through and
handled now.
my_str_malloc_default() did exit(1) on error, replaced my_str_ allocator
functions with normal my_malloc()/my_realloc()/my_free().
sql_connect.cc did many exit(1) on hash initialisation failure. Removed error
check since my_hash_init() never fails.
my_malloc() did exit(1) on error. Replaced with abort().
my_load_defaults() did exit(1) on error, replaced with return 2.
my_load_defaults() still does exit(0) when invoked with --print-defaults.
