Skip to content

Remove assert() instances from libkmod #382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Remove assert() instances from libkmod #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
63547ab
libkmod: remove unreachable assert() instances
evelikov Jun 26, 2025
b7339f4
libkmod: annotate index_mm_open() as _nonnull_all_
evelikov Jun 26, 2025
d5499e3
libkmod: remove assert() from public API
evelikov Jun 26, 2025
e8cab7c
libkmod: pass the command to module_do_install_commands()
evelikov Jun 26, 2025
bd7c765
libkmod: remove unreachable asserts in kmod_module_get_probe_list()
evelikov Jun 26, 2025
afe8f96
libkmod: remove unreachable assert in kmod_module_parse_depline()
evelikov Jun 26, 2025
9d75362
libkmod: remove unreachable assert from kmod_lookup_alias_from_*
evelikov Jun 26, 2025
d361b5f
shared/array: remove unreachable assert
evelikov Jun 26, 2025
e2a7495
shared/strbuf: annotate strbuf_pushmem() as _nonnull_all_
evelikov Jun 26, 2025
b9137f9
shared/strbuf: remove remaining assert() instances
evelikov Jun 26, 2025
4e2417e
testsuite: remove asserts(), fix create_sysfs_files errorpath
evelikov Jun 26, 2025
b49c0b4
Use ISO/C11 static_assert
evelikov Jun 26, 2025
7f504cb
Remove no-longer needed assert.h includes
evelikov Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions libkmod/libkmod-elf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* Copyright (C) 2011-2013 ProFUSION embedded systems
*/

#include <assert.h>
#include <elf.h>
#include <endian.h>
#include <limits.h>
Expand Down Expand Up @@ -146,8 +145,6 @@ static inline uint64_t elf_get_uint(const struct kmod_elf *elf, uint64_t offset,
const uint8_t *p;
uint64_t ret = 0;

assert(size <= sizeof(uint64_t));

p = elf->memory + offset;

if (elf->msb) {
Expand All @@ -174,8 +171,6 @@ static inline int elf_set_uint(const struct kmod_elf *elf, uint64_t offset, uint
"size=%" PRIu64 " offset=%" PRIu64 " value=%" PRIu64 " write memory=%p\n",
size, offset, value, changed);

assert(size <= sizeof(uint64_t));

p = changed + offset;
if (elf->msb) {
for (i = 1; i <= size; i++) {
Expand Down Expand Up @@ -204,8 +199,6 @@ static inline const void *elf_get_mem(const struct kmod_elf *elf, uint64_t offse
static inline uint64_t elf_get_section_header_offset(const struct kmod_elf *elf,
uint16_t idx)
{
assert(idx != SHN_UNDEF);
assert(idx < elf->header.section.count);
if (idx == SHN_UNDEF || idx >= elf->header.section.count) {
ELFDBG(elf, "invalid section number: %" PRIu16 ", last=%" PRIu16 "\n",
idx, elf->header.section.count);
Expand Down Expand Up @@ -662,8 +655,6 @@ int kmod_elf_strip(const struct kmod_elf *elf, unsigned int flags, const void **
uint8_t *changed;
int err = 0;

assert(flags & (KMOD_INSERT_FORCE_MODVERSION | KMOD_INSERT_FORCE_VERMAGIC));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mixed feelings with these assert() removals. They were actually added for programming mistakes. Not for input errors handling. For this case, the input control is in do_init_module().

The assert here is because the function only handles KMOD_INSERT_FORCE_MODVERSION and KMOD_INSERT_FORCE_VERMAGIC. If we passed flags that doesn't contain those, it's not the end of the world: the function will continue to do its job, but it's likely wrong: it would mean we probably added another flag and forgot to add its handling in this function. By removing it we are making it silent rather than simply crash the program/test during development.

So IMO the statement "we shouldn't have asserts in a shared library" is wrong. It shouldn't be used for input control or error handling, but it's still useful to catch program programming mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And true, some of them shouldn't be there and we should add the error handling, but I will need to review case by case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to keep some for the short run, until we get more extensive tests to cover the code-paths, or you prefer to keep them even in that case?

As a whole, it feels slightly awkward having them available (potentially reachable) in distribution builds. Perhaps they can be "enabled" with debug-messages or another meson toggle?

Fwiw most distributions are not* be using debug-messages these days.

  • Fedora should stop using it any day now. While Gentoo/Yocto have it accessible with their modular build setups.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For enabling/disabling maybe we should have this instead?

diff --git a/meson.build b/meson.build
index 5740e13..75343ed 100644
--- a/meson.build
+++ b/meson.build
@@ -7,6 +7,7 @@ project(
   default_options : [
     'c_std=gnu11',
     'b_pie=true',
+    'b_ndebug=if-release',
     'warning_level=2',
     'prefix=/usr',
     'sysconfdir=/etc',

... to be merged after the wrong asserts are removed, so we still pass the testsuite tests.

Distros can also control it with -Db_ndebug=...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm correctly parsing the meson documentation this is the default already. The issue is that due to $reasons, at least some distributions are building with an explicit --build-type plain.

At a glance we can easily swap the assert(static literal...) checks for assert_cc()` as below. It seems like the better option IMHO since it's a compile-time check. What do you think?

diff --git a/shared/array.c b/shared/array.c
index 85f8955e..da40d36e 100644
--- a/shared/array.c
+++ b/shared/array.c
@@ -36,7 +36,7 @@ static void array_trim(struct array *array)
 	}
 }
 
-void array_init(struct array *array, size_t step)
+void __array_init(struct array *array, size_t step)
 {
 	array->array = NULL;
 	array->count = 0;
diff --git a/shared/array.h b/shared/array.h
index 8facc52d..1cdcaed0 100644
--- a/shared/array.h
+++ b/shared/array.h
@@ -13,7 +13,10 @@ struct array {
 	size_t step;
 };
 
-void array_init(struct array *array, size_t step);
+void __array_init(struct array *array, size_t step);
+#define array_init(a, s) \
+	assert_cc(s > 0); \
+	__array_init(a, s);
 int array_append(struct array *array, const void *element);
 int array_append_unique(struct array *array, const void *element);
 void array_pop(struct array *array);


changed = memdup(elf->memory, elf->size);
if (changed == NULL)
return -ENOMEM;
Expand Down
3 changes: 0 additions & 3 deletions libkmod/libkmod-index.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include <sys/param.h>

#include <arpa/inet.h>
#include <assert.h>
#include <errno.h>
#include <fnmatch.h>
#include <inttypes.h>
Expand Down Expand Up @@ -752,8 +751,6 @@ int index_mm_open(const struct kmod_ctx *ctx, const char *filename,
} hdr;
const void *p;

assert(pidx != NULL);

DBG(ctx, "file=%s\n", filename);

idx = malloc(sizeof(*idx));
Expand Down
4 changes: 2 additions & 2 deletions libkmod/libkmod-index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ void index_values_free(struct index_value *values);

/* Implementation using mmap */
struct index_mm;
int index_mm_open(const struct kmod_ctx *ctx, const char *filename,
unsigned long long *stamp, struct index_mm **pidx);
_nonnull_all_ int index_mm_open(const struct kmod_ctx *ctx, const char *filename,
unsigned long long *stamp, struct index_mm **pidx);
void index_mm_close(struct index_mm *index);
char *index_mm_search(const struct index_mm *idx, const char *key);
struct index_value *index_mm_searchwild(const struct index_mm *idx, const char *key);
Expand Down
40 changes: 10 additions & 30 deletions libkmod/libkmod-module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* Copyright (C) 2011-2013 ProFUSION embedded systems
*/

#include <assert.h>
#include <ctype.h>
#include <dirent.h>
#include <errno.h>
Expand Down Expand Up @@ -117,7 +116,7 @@ void kmod_module_parse_depline(struct kmod_module *mod, char *line)

if (mod->init.dep)
return;
assert(mod->dep == NULL);

mod->init.dep = true;

p = strchr(line, ':');
Expand Down Expand Up @@ -805,17 +804,14 @@ struct probe_insert_cb {
void *data;
};

static int module_do_install_commands(struct kmod_module *mod, const char *options,
struct probe_insert_cb *cb)
static int module_do_install_commands(struct kmod_module *mod, const char *command,
const char *options, struct probe_insert_cb *cb)
{
const char *command = kmod_module_get_install_commands(mod);
char *p;
_cleanup_free_ char *cmd;
int err;
size_t cmdlen, options_len, varlen;

assert(command);

if (options == NULL)
options = "";

Expand Down Expand Up @@ -981,9 +977,6 @@ static int kmod_module_get_probe_list(struct kmod_module *mod, bool ignorecmd,
{
int err;

assert(mod != NULL);
assert(list != NULL && *list == NULL);

/*
* Make sure we don't get screwed by previous calls to this function
*/
Expand Down Expand Up @@ -1073,7 +1066,7 @@ KMOD_EXPORT int kmod_module_probe_insert_module(
print_action(m, true, options ?: "");

if (!(flags & KMOD_PROBE_DRY_RUN))
err = module_do_install_commands(m, options, &cb);
err = module_do_install_commands(m, cmd, options, &cb);
} else {
if (print_action != NULL)
print_action(m, false, options ?: "");
Expand Down Expand Up @@ -1243,12 +1236,9 @@ KMOD_EXPORT int kmod_module_get_softdeps(const struct kmod_module *mod,
const struct kmod_list *l;
const struct kmod_config *config;

if (mod == NULL || pre == NULL || post == NULL)
if (mod == NULL || pre == NULL || *pre != NULL || post == NULL || *post != NULL)
return -ENOENT;

assert(*pre == NULL);
assert(*post == NULL);

config = kmod_get_config(mod->ctx);

kmod_list_foreach(l, config->softdeps) {
Expand Down Expand Up @@ -1280,11 +1270,9 @@ KMOD_EXPORT int kmod_module_get_weakdeps(const struct kmod_module *mod,
const struct kmod_list *l;
const struct kmod_config *config;

if (mod == NULL || weak == NULL)
if (mod == NULL || weak == NULL || *weak != NULL)
return -ENOENT;

assert(*weak == NULL);

config = kmod_get_config(mod->ctx);

kmod_list_foreach(l, config->weakdeps) {
Expand Down Expand Up @@ -1858,11 +1846,9 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod,
int i, count, ret = -ENOMEM;
struct kmod_signature_info sig_info = {};

if (mod == NULL || list == NULL)
if (mod == NULL || list == NULL || *list != NULL)
return -ENOENT;

assert(*list == NULL);

/* remove const: this can only change internal state */
if (kmod_module_is_builtin((struct kmod_module *)mod)) {
count = kmod_builtin_get_modinfo(mod->ctx, kmod_module_get_name(mod),
Expand Down Expand Up @@ -2013,11 +1999,9 @@ KMOD_EXPORT int kmod_module_get_versions(const struct kmod_module *mod,
struct kmod_modversion *versions;
int i, count, ret = 0;

if (mod == NULL || list == NULL)
if (mod == NULL || list == NULL || *list != NULL)
return -ENOENT;

assert(*list == NULL);

ret = kmod_module_get_elf(mod, &elf);
if (ret)
return ret;
Expand Down Expand Up @@ -2114,11 +2098,9 @@ KMOD_EXPORT int kmod_module_get_symbols(const struct kmod_module *mod,
struct kmod_modversion *symbols;
int i, count, ret = 0;

if (mod == NULL || list == NULL)
if (mod == NULL || list == NULL || *list != NULL)
return -ENOENT;

assert(*list == NULL);

ret = kmod_module_get_elf(mod, &elf);
if (ret)
return ret;
Expand Down Expand Up @@ -2220,11 +2202,9 @@ KMOD_EXPORT int kmod_module_get_dependency_symbols(const struct kmod_module *mod
struct kmod_modversion *symbols;
int i, count, ret = 0;

if (mod == NULL || list == NULL)
if (mod == NULL || list == NULL || *list != NULL)
return -ENOENT;

assert(*list == NULL);

ret = kmod_module_get_elf(mod, &elf);
if (ret)
return ret;
Expand Down
11 changes: 0 additions & 11 deletions libkmod/libkmod.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* Copyright (C) 2011-2013 ProFUSION embedded systems
*/

#include <assert.h>
#include <ctype.h>
#include <errno.h>
#include <fnmatch.h>
Expand Down Expand Up @@ -359,8 +358,6 @@ static int kmod_lookup_alias_from_alias_bin(struct kmod_ctx *ctx,
struct index_file *idx;
struct index_value *realnames, *realname;

assert(*list == NULL);

if (ctx->indexes[index_number] != NULL) {
DBG(ctx, "use mmapped index '%s' for name=%s\n",
index_files[index_number].fn, name);
Expand Down Expand Up @@ -485,8 +482,6 @@ int kmod_lookup_alias_from_kernel_builtin_file(struct kmod_ctx *ctx, const char
int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name,
struct kmod_list **list)
{
assert(*list == NULL);

if (lookup_builtin_file(ctx, name)) {
struct kmod_module *mod;
struct kmod_list *node;
Expand Down Expand Up @@ -530,8 +525,6 @@ int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char *name,
char *line;
int n = 0;

assert(*list == NULL);

/*
* Module names do not contain ':'. Return early if we know it will
* not be found.
Expand Down Expand Up @@ -575,8 +568,6 @@ int kmod_lookup_alias_from_config(struct kmod_ctx *ctx, const char *name,
struct kmod_list *l;
int err, nmatch = 0;

assert(*list == NULL);

kmod_list_foreach(l, config->aliases) {
const char *aliasname = kmod_alias_get_name(l);
const char *modname = kmod_alias_get_modname(l);
Expand Down Expand Up @@ -619,8 +610,6 @@ int kmod_lookup_alias_from_commands(struct kmod_ctx *ctx, const char *name,
struct kmod_list *l, *node;
int err, nmatch = 0;

assert(*list == NULL);

kmod_list_foreach(l, config->install_commands) {
const char *modname = kmod_command_get_modname(l);

Expand Down
1 change: 0 additions & 1 deletion meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ foreach tuple : _decls
cdata.set10('HAVE_DECL_@0@'.format(decl.to_upper()), have)
endforeach

cdata.set10('HAVE_STATIC_ASSERT', cc.compiles('_Static_assert(1, "Test");', name : '_Static_assert'))
cdata.set10('HAVE_NORETURN', cc.compiles('#include <stdlib.h>; _Noreturn int foo(void) { exit(0); }', name : '_Noreturn'))

################################################################################
Expand Down
2 changes: 0 additions & 2 deletions shared/array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* Copyright (C) 2011-2013 ProFUSION embedded systems
*/

#include <assert.h>
#include <errno.h>
#include <stdint.h>
#include <stdlib.h>
Expand Down Expand Up @@ -39,7 +38,6 @@ static void array_trim(struct array *array)

void array_init(struct array *array, size_t step)
{
assert(step > 0);
array->array = NULL;
array->count = 0;
array->total = 0;
Expand Down
10 changes: 2 additions & 8 deletions shared/macro.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,10 @@
*/
#pragma once

#include <assert.h>
#include <stddef.h>

#if HAVE_STATIC_ASSERT
#define assert_cc(expr) _Static_assert((expr), #expr)
#else
#define assert_cc(expr) \
do { \
(void)sizeof(char[1 - 2 * !(expr)]); \
} while (0)
#endif
#define assert_cc(expr) static_assert((expr), #expr)

#define check_types_match(expr1, expr2) ((typeof(expr1) *)0 != (typeof(expr2) *)0)

Expand Down
7 changes: 0 additions & 7 deletions shared/strbuf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
* Copyright (C) 2014 Intel Corporation. All rights reserved.
*/

#include <assert.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
Expand Down Expand Up @@ -83,9 +82,6 @@ bool strbuf_pushchar(struct strbuf *buf, char ch)

size_t strbuf_pushmem(struct strbuf *buf, const char *src, size_t sz)
{
assert(src != NULL);
assert(buf != NULL);

if (sz == 0)
return 0;

Expand All @@ -100,19 +96,16 @@ size_t strbuf_pushmem(struct strbuf *buf, const char *src, size_t sz)

void strbuf_popchar(struct strbuf *buf)
{
assert(buf->used > 0);
buf->used--;
}

void strbuf_popchars(struct strbuf *buf, size_t n)
{
assert(buf->used >= n);
buf->used -= n;
}

void strbuf_shrink_to(struct strbuf *buf, size_t sz)
{
assert(buf->used >= sz);
buf->used = sz;
}

Expand Down
2 changes: 1 addition & 1 deletion shared/strbuf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ void strbuf_clear(struct strbuf *buf);
const char *strbuf_str(struct strbuf *buf);

bool strbuf_pushchar(struct strbuf *buf, char ch);
size_t strbuf_pushmem(struct strbuf *buf, const char *src, size_t sz);
_nonnull_all_ size_t strbuf_pushmem(struct strbuf *buf, const char *src, size_t sz);
static inline size_t strbuf_pushchars(struct strbuf *buf, const char *str)
{
return strbuf_pushmem(buf, str, strlen(str));
Expand Down
1 change: 0 additions & 1 deletion shared/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* Copyright (C) 2013-2014 Intel Corporation. All rights reserved.
*/

#include <assert.h>
#include <ctype.h>
#include <dlfcn.h>
#include <errno.h>
Expand Down
1 change: 0 additions & 1 deletion testsuite/delete_module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* Copyright (C) 2012-2013 ProFUSION embedded systems
*/

#include <assert.h>
#include <dirent.h>
#include <dlfcn.h>
#include <errno.h>
Expand Down
Loading
Loading