Coverity wrongly think there is a use after free in the test:
Error: USE_AFTER_FREE (CWE-416): [#def559] [important]
NetworkManager-1.31.90/src/libnm-glib-aux/tests/test-shared-general.c:1305: alias: Assigning: "s1" = "_s". Now both point to the same storage.
NetworkManager-1.31.90/src/libnm-glib-aux/tests/test-shared-general.c:1324: freed_arg: "g_source_unref" frees "s1".
NetworkManager-1.31.90/src/libnm-glib-aux/tests/test-shared-general.c:1330: deref_after_free: Dereferencing freed pointer "s1".
# 1328| s2 = nm_g_source_sentinel_get(0);
# 1329| g_assert(s2 == s1);
# 1330|-> g_assert_cmpint(g_atomic_int_get(&s1->ref_count), >=, 1);
# 1331| }
# 1332| }
Rework the code in the hope to avoid the false warning.
(cherry picked from commit 7825609f1f)
(cherry picked from commit c47c823c9d)
This helper is useful to get a dummy GSource instance that can be
refed, unrefed and destroyed. It can act as a replacement for
a timeout source with infinite timeout.
(cherry picked from commit ce7c28c514)
The change broke unit tests on 32 bit systems.
Change the code again to make it more similar to what it was
before. Now only on 64 bit systems there is any difference compared
to before. That makes it easier about reasoning for how the unit test
should be (in most cases, it is unchanged).
Fixes: 040c86f15c ('shared: avoid compiler warning for nm_utils_get_next_realloc_size() returning huge sizes')
On s390x (gcc-8.3.1-5.1.el8.s390x) the compiler warns that we don't
pass size larger than 2^63-1 to malloc. With LTO enabled, it is also
quite adamant in detecting that with nm_utils_get_next_realloc_size().
Optimally, we would disable this useless warning with "-Wno-alloc-size-larger-than",
but that seems not to work. So add a workaround in code :(
It's hard to actually workaround the warning while handling all kinds of
sizes. The only simple solution is to no handle such huge cases and only
assert.
In function 'nm_secret_mem_realloc',
inlined from '_nm_str_buf_ensure_size' at shared/nm-glib-aux/nm-shared-utils.c:5316:31:
shared/nm-glib-aux/nm-secret-utils.h:180:17: error: argument 1 value '18446744073709551615' exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
m_new = g_malloc(new_len);
^
shared/nm-glib-aux/nm-secret-utils.h: In function '_nm_str_buf_ensure_size':
/usr/include/glib-2.0/glib/gmem.h:78:10: note: in a call to allocation function 'g_malloc' declared here
gpointer g_malloc (gsize n_bytes) G_GNUC_MALLOC G_GNUC_ALLOC_SIZE(1);
^
lto1: all warnings being treated as errors
We have:
- nm_utils_hashtable_cmp(): this does a full cmp of two hash
tables, with the intent to provide a stable sort order.
It thus takes a GCompareDataFunc() argument.
- nm_utils_hashtable_cmp_equal(): this is like nm_utils_hashtable_cmp(),
except that the caller won't get a compare value, only a boolean
value that indicates equality.
This was previously called nm_utils_hashtable_equal().
- nm_utils_hashtable_equal(): this takes a GEqualFunc function
for comparing the values for equality. It takes thus
a different kind of predicate, but otherwise is similar to
nm_utils_hashtable_cmp_equal().
This was previously called nm_utils_hash_table_equal().
Unify the naming of these functions.
The underscore somehow indicated that these would be an internal
function. Which they are in the sense that they are in "shared/nm-glib-aux/".
But they part of our internal helper functions, and in our code base
their use is no discouraged or "private.
Also, next I'll replace the function call with a macro, so, I will
have a need for the underscore name.
Rename.
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
nm_utils_hexstr2bin_full() is our general hexstr to binary parsing
method. It uses (either mandatory or optional) delimiters. Before,
if delimiters are in use, it would accept individual hexdigits.
E.g. "a:b" would be accepted as "0a:0b:.
Add an argument that prevents accepting such single digits.
This makes the macro more function like. Also, taking a pointer
makes it a bit clearer that this possibly changes the value.
Of course, it's not a big difference to before, but this
form seems slightly preferable to me.
nm_utils_is_specific_hostname() is basically to check whether the
hostname is localhost (and also handle "(null)").
In that sense, it's similar to systemd's is_localhost(). Extend or
variant to
- be case insensitive (like is_localhost()).
- accept more variants of localhost/localdomain names as special.
Our "nm-json-aux.h" redefines various things from <jansson.h> header.
Add a unit test that checks that what we redefine exactly matches what
libjansson would provide, so that they are compatible.
- add unit test for nm_utils_parse_next_line()
- as line delimiter also accept "\r\n" and "\r" (beside "\n", "\0" and
EOF).
- fix returning lines with embedded "\0" characters. The line ends
on the first "\n" or "\0", whatever comes first. The code before
didn't ensure that with:
line_end = memchr (line_start, '\n', *inout_len);
if (!line_end)
line_end = memchr (line_start, '\0', *inout_len);
When we have a buffer that we want to grow exponentially with
nm_utils_get_next_realloc_size(), then there are certain buffer
sizes that are better suited.
For example, if you have an empty NMStrBuf (len == 0), and you
want to allocate roughly one kilobyte, then 1024 is a bad choice,
because nm_utils_get_next_realloc_size() will give you 2024 bytes.
NM_UTILS_GET_NEXT_REALLOC_SIZE_1000 might be better in this case.
When growing a buffer by appending a previously unknown number
of elements, the often preferable strategy is growing it exponentially,
so that the amortized runtime and re-allocation costs scale linearly.
GString just always increases the buffer length to the next power of
two. That works.
I think there is value in trying to find an optimal next size. Because
while it doesn't matter in terms of asymptotic behavior, in practice
a better choice should make a difference. This is inspired by what QT
does ([1]), to take more care when growing the buffers:
- QString allocates 4 characters at a time until it reaches size 20.
- From 20 to 4084, it advances by doubling the size each time. More
precisely, it advances to the next power of two, minus 12. (Some memory
allocators perform worst when requested exact powers of two, because
they use a few bytes per block for book-keeping.)
- From 4084 on, it advances by blocks of 2048 characters (4096 bytes).
This makes sense because modern operating systems don't copy the entire
data when reallocating a buffer; the physical memory pages are simply
reordered, and only the data on the first and last pages actually needs
to be copied.
Note that a QT is talking about 12 characters, so we use 24 bytes
head room.
[1] https://doc.qt.io/qt-5/containers.html#growth-strategies
NMTST_SWAP() used memcpy() for copying the value, while NM_SWAP() uses
a temporary variable with typeof(). I think the latter is preferable.
Also, the macro is essentially doing the same thing.
Several macros are used to define function. They had a "_STATIC" variant,
to define the function as static.
I think those macros should not try to abstract entirely what they do.
They should not accept the function scope as argument (or have two
variants per scope). This also because it might make sense to add
additional __attribute__(()) to the function. That only works, if
the macro does not pretend to *not* define a plain function.
Instead, embrace what the function does and let the users place the
function scope as they see fit.
This also follows what is already done with
static NM_CACHED_QUARK_FCN ("autoconnect-root", autoconnect_root_quark)
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
"shared/nm-utils" got long renamed and split into separate parts. The remaining
tests are really to test nm-std-aux and nm-glib-aux (no libnm dependencies). Move
the tests to the appropriate place.
