- existing API/ABI must stay unchanged. We cannot change parameter
types. Ad most we can add new variants that support 64 bit integers.
- rtnl_tc_calc_txtime64() and rtnl_tc_calc_bufsize64() are trivial.
We should not blow up the public API of libnl for such a thing.
If the users needs it, they can just reimplement it.
- getters should return an error code. Especially if the return type
does not support encoding an error there.
- don't add separate rs_rate64/rs_ceil64 field. Instead, extend the
"rs_rate" field of "struct rtnl_ratespec" to 64 bits. It's internal
API.
Returning the value directly as uint32_t does not leave room for an error
code. E.g. we want to indicate to the caller whether the attribute is present
or not (-NLE_NOATTR). Currenlty, the code is quite unforgiving and will just
crash/assert against invalid arguments. In theory, we could also be more forgiving
and return a error code if the link argument is invalid.
Just be more forgiving. Also, this avoids a coverity warning:
Error: FORWARD_NULL (CWE-476): [#def1]
libnl-3.4.0/lib/route/addr.c:502: var_compare_op: Comparing "a->a_peer" to null implies that "a->a_peer" might be null.
libnl-3.4.0/lib/route/addr.c:513: var_deref_model: Passing null pointer "a->a_peer" to "nl_addr_cmp", which dereferences it.
libnl-3.4.0/lib/addr.c:587:8: deref_parm: Directly dereferencing parameter "a".
# 585| int nl_addr_cmp(const struct nl_addr *a, const struct nl_addr *b)
# 586| {
# 587|-> int d = a->a_family - b->a_family;
# 588|
# 589| if (d == 0) {
https://bugzilla.redhat.com/show_bug.cgi?id=1606988
This fixes the build with musl libc.
Additionally, several changes were made to account for changes to the
headers:
- ip_mp_alg.h was removed, since it was removed in linux commit e06e7c61
(v2.6.23), and the last use of those constants was removed in libnl
commit 535e8316.
- Uses of TCF_META_ID_SK_ROUTE_CAPS were updated to
__TCF_META_ID_SK_ROUTE_CAPS, since it was renamed in linux commit
e20e6940 (v3.1).
- Uses of IF_CARRIER_DOWN and IF_CARRIER_UP were replaced with their
values, 0 and 1, since they are not in linux/if.h (they appear to be
libnl-specific, added in libnl commit 3540e44b).
https://github.com/thom311/libnl/pull/222
These behave the same, except when used at top-level. This can't happen
since the macro body is a statement.
__func__ is standardized since C99, while __PRETTY_FUNCTION__ is a
GNU extension.
The compiler warns about string truncation:
In function ‘genl_family_add_grp’,
inlined from ‘family_clone’ at lib/genl/family.c:81:9,
inlined from ‘family_clone’ at lib/genl/family.c:66:12:
lib/genl/family.c:376:2: error: ‘strncpy’ output may be truncated copying 15 bytes from a string of length 15 [-Werror=stringop-truncation]
376 | strncpy(grp->name, name, GENL_NAMSIZ - 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Obvioulsy, it's a bug to use an invalid group name. But better
handle it by checking for a suitable string length.
Also use _nl_strncpy() which asserts that no truncation occurs.
The compiler warns:
In function ‘rtnl_tc_set_kind’,
inlined from ‘rtnl_tc_msg_parse’ at lib/route/tc.c:81:2:
lib/route/tc.c:532:2: error: ‘strncpy’ output may be truncated copying 31 bytes from a string of length 31 [-Werror=stringop-truncation]
532 | strncpy(tc->tc_kind, kind, sizeof(tc->tc_kind) - 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Now, there are two choices: either accept the truncation
or rejecting it.
While rejecting it is a change in behavior and API, I don't think that
any caller actually relied on that. That is because such "kind" name would
be invalid anyway (and rejected from kernel too).
So, tighten up the API and check for a suitable string length.
Also, use _nl_strncpy() instead of strncpy(). Note that that doesn't suppress
the warning, it merely (also) adds an _nl_assert() for something that already
shouldn't happen.
Compiler warnings:
lib/route/link/inet6.c: In function ‘inet6_dump_details’:
lib/route/link/inet6.c:383:3: error: ‘strncpy’ output may be truncated copying between 0 and 63 bytes from a string of length 63 [-Werror=stringop-truncation]
383 | strncpy(&buf[offset], buf2, strlen(buf2));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Also, passing "strlen(buf2)" as length argument to strncpy() is
certainly wrong.
The follow leaves a dangling pointer when the name argument is too long:
xfrmnl_sa_set_aead_params:
if (sa->aead)
free (sa->aead);
if ( strlen (alg_name) >= sizeof (sa->aead->alg_name)
|| (sa->aead = calloc (1, newlen)) == NULL)
return -1;
Fix that, but do more:
- ensure that we don't modify the object when the setter is going to
fail. That means, first check whether we can succeed with all the
steps that are requested, and (in case we cannot) fail without
modifing the target object.
- bonus points for making the setter self-assignment safe by reordering
the setting and freeing of the memory.
We want to build with -Wvla, because VLAs interfere with static asserts
(if the condition of a static assert is not actually static, then VLAs
make it silently pass).
Also, VLAs should be avoided because we want to be in contol how much
we allocate on the stack.
- don't leave a dangling pointer, in case we unset the
kind.
- try first to clone the string. If that fails, return early
without modifying the link. Only start modifying the link,
after we know it's going to succeed.
A detailed explanation is provided in the original Linux kernel commit that
fixes the bug: 1045b03e07d85f3545118510a587035536030c1c
Valgrind spotted the issue when the remaining was negative.
This bug was triggering application crashes.
Signed-off-by: Patrick Havelange <patrick.havelange@tessares.net>
https://github.com/thom311/libnl/pull/199
rtnl_act_append() cannot add more than TCA_ACT_MAX_PRIO actions to the
same list. Because of that rtnl_basic_add_action() and
rtnl_u32_add_action() should not increment the reference counter of the
given action until it is successfully added to the filter's list.
Signed-off-by: Ilya Pronin <ipronin@twitter.com>
Fixes: e5d9b828f6https://github.com/thom311/libnl/pull/201
Our API is unfortunately not consistent about this.
However, in general, getters should aim to return an
error code whether the attribute could be retrieved.
Our API is unfortunately not consistent about this.
However, in general, getters should aim to return an
error code whether the attribute could be retrieved.
BUG() raises an assertion. It seems overly harsh.
For example, rtnl_tc_data() can fail if we fail to allocate
memory. Asserting against that, makes libnl3 not out-of-memory
safe.
Just return a regular error.
