invalid pointer pairs are when the length/memory of one string are
intermixed with another.
For comp_err, the end null pointer was compared to soffset within
my_strtoll10. As we didn't need the end position a NULL arg option
was compatble.
For uca-dump, Address Sanitizer raises invalid pointer pair because
argv options (opt) don't have an obvious correlation of having a start
at opt_X.length, even though the lstrncmp makes this true.
The DBUG_ASSERTS of strmov (added MDEV-11752) where incompatible with
pointer-compare. Replaced strmov with static inline version in
m_string.h using memmove that allows overlaps, and being inline allows
the uneeded parts of the implementation to be optimized away.
Clang processes the "int x=x" code from UNINIT_VAR
literally resulting in an uninitialized read and write.
This is something we want to avoid. Gcc does the same
without emitting warnings.
As the UNINIT_VAR was around avoiding compiler false detection,
and clang doesn't false detect, is default action is a
noop.
Static analysers (examined Infer and SonarQube) are
clang based and have the same detection.
Using a __clang__ instead of WITH_UBSAN would acheived
a better result, however reviewer wanted to keep WITH_UBSAN
only.
LINT_INIT_STRUCT is no longer required, even a gcc-4.8.5
doesn't warn with this construct removed which matches
the comment that it was fixed in gcc ~4.7.
mysql.cc - all paths in com_go populate buff before use.
json: Item_func_json_merge::val_str
LINT_INIT(js2) unneeded as usage in the previous statements
it is explicitly initialized to NULL.
Item_func_json_contains_path::val_bool n_found is guarded
by an uninitialized read by mode_one and from
gcc-13.3.0 in Ubuntu 24.04 this is detected. As the only
remaining use of LINIT_INIT this usage has been applied
with the expanded macro with the unused _lint define removed.
The LINT_INIT macro is removed.
_ma_ck_delete - org_key only valid under share->now_transactional
likewise with _ma_ck_write_btree_with_log
connect engine never used anything that FORCE_INIT_OF_VARS
would change.
Reviewer: Monty
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.
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.
