Reduce strerror and errno usage #346

evelikov · 2025-05-10T15:27:08Z

Originally inspired by #244, with some commit (fragments) from #336 and #343

Tl:Dr: we no longer manually set errno but propagate the error code manually, iff needed. In addition most (but not all) of our strerror() usage is replaced by %m.

As a nice side-effect the size of libkmod.so (should also be applicable for the tools) has decreased:

text data bss dec hex filename
116099 2224 8 118331 1ce3b before//libkmod.so
115685 2224 8 117917 1cc9d after/libkmod.so

NOTE: the PR also includes a few related bug fixes of varying severity. Happy to flesh them out into separate PR/PRs if that helps.

libkmod/libkmod-module.c

-		mod->file = kmod_file_open(mod->ctx, path);
-		if (mod->file == NULL) {
-			err = -errno;
+		err = kmod_file_open(mod->ctx, path, &mod->file);


libkmod/libkmod-module.c

-		((struct kmod_module *)mod)->file = kmod_file_open(mod->ctx, path);
-		if (mod->file == NULL)
-			return NULL;
+		ret = kmod_file_open(mod->ctx, path, &((struct kmod_module *)mod)->file);


evelikov · 2025-05-11T12:26:48Z

The code-base is somewhat inconsistent - some places use "if (ret < 0)" others simply "if (ret)". I've tried to follow the surrounding pattern with the new patches, although I don't have a strong preference either way.

libkmod/libkmod-builtin.c

testsuite/init_module.c

codecov · 2025-05-18T21:57:15Z

Codecov Report

Attention: Patch coverage is 43.95604% with 51 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
libkmod/libkmod-module.c	36.66%	9 Missing and 10 partials ⚠️
libkmod/libkmod-builtin.c	38.46%	5 Missing and 3 partials ⚠️
libkmod/libkmod-elf.c	46.15%	4 Missing and 3 partials ⚠️
libkmod/libkmod-file.c	66.66%	3 Missing and 4 partials ⚠️
testsuite/init_module.c	14.28%	4 Missing and 2 partials ⚠️
tools/modinfo.c	60.00%	1 Missing and 1 partial ⚠️
testsuite/delete_module.c	0.00%	1 Missing ⚠️
tools/depmod.c	0.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
libkmod/libkmod-file-zlib.c	`55.26% <ø> (ø)`
shared/util.c	`65.95% <ø> (ø)`
testsuite/delete_module.c	`70.58% <0.00%> (ø)`
tools/depmod.c	`56.99% <0.00%> (ø)`
tools/modinfo.c	`49.60% <60.00%> (ø)`
testsuite/init_module.c	`68.08% <14.28%> (ø)`
libkmod/libkmod-elf.c	`50.93% <46.15%> (ø)`
libkmod/libkmod-file.c	`76.81% <66.66%> (ø)`
libkmod/libkmod-builtin.c	`60.24% <38.46%> (ø)`
libkmod/libkmod-module.c	`54.02% <36.66%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lucasdemarchi · 2025-05-19T14:26:45Z

libkmod/libkmod-file.c


 	file->fd = open(filename, O_RDONLY | O_CLOEXEC);
 	if (file->fd < 0) {
 		free(file);
-		return NULL;
+		return -ENOMEM;


ENOMEM here doesn't feel right. However using the errno from open() after the free() is also invalid. Options: a) add a local saved_errno in the error path so you can return it; b) annotate file with cleanup_free, and assign it to NULL after setting *out_file. Approach (b) would allow to remove one more free() call in the error path.

using the errno from open() after the free() is also invalid

Not sure I follow - why is it invalid? The free() call should never alter errno... does it?

Either way, option b) applied. Thanks for the review.

Not sure I follow - why is it invalid? The free() call should never alter errno... does it?

humn.. true

Either way, option b) applied. Thanks for the review.

thanks

Currently, the function will report the exact same issue twice. Where the second DBG() and the error code returned, should highlight the (potential) stat(2) failure. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

The manual page of strerror() outlines a number of caveats wrt its usage. Swap for the GNU specific %m printf modifier (also supported on musl and bionic), which side-steps the issues as much as possible. Co-authored-by: Cristian Rodríguez <cristian@rodriguez.im> [emil: split from larger patch, convert more instances to %m] Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

Currently we reset errno prior to calling strto{u,}l(). This is not needed, since a) we don't check errno to see if the function was successful and b) we explicitly propagate the error code by returning it directly to the caller. Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

Currently we reset errno, shortly to be followed by an fcntl() and gzdopen() calls. Both of those should set errno on failure and preserve it on success. Just leave errno as-is, we shouldn't be changing it in this context. Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

The single caller of add_param() does not need the struct param*, so we might as well return the error code directly. As a result we don't manually overwrite errno. Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

Change the function signature (bool -> int) and directly return the error code. Thus we no longer need to overwrite errno. v2: - return false -> return -ENAMETOOLONG Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

Currently our wrapper init_module() will happily return success whenever libkmod fails. While such failures are unlikely, our wrapper should also fail. In part so it doesn't mask a potentially deeper problem and in part because the kmod API used, will set errno... Something a normal syscall wouldn't do AFAICT. v2: - remove the respective comment Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

The function does bounds checking, allocation and copying. In the first instance, we manually set errno (to ENOMEM?) on failure. The realloc() call does the same, implicitly. In practice we don't distinguish between the two failures, so we might as well stop manually setting errno and always assume ENOMEM in the caller. Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

As per the manual getdelim(3): The buffer is null-terminated and ... Remove the local check and inline the function call. As a nice result, we no longer set the errno and the context of feof() is obvious. Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

Rework the function signature and return the error code instead of the stripped module. Thus we no longer explicitly set errno. Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

In practise none of our code-paths will use a NULL pointer so we might as well enforce that. To stay aligned with the kernel behaviour update our init_module() preload library to return EFAULT... Should we get confused and pass NULL in the future. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

The function returns what is considered constant data and all the callers handle it as such. Add the declaration to make things obvious. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

Rework the function signature and return the error code instead of the stripped module. Thus we no longer explicitly set errno. Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

Rework the function signature and return the error code instead of the stripped module. Thus we no longer explicitly set errno. v2: - kmod_file_open() - use _cleanup_free_, return errno instead of ENOMEM Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

Currently, the function will report the exact same issue twice. Where the second DBG() and the error code returned, should highlight the (potential) stat(2) failure. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

The manual page of strerror() outlines a number of caveats wrt its usage. Swap for the GNU specific %m printf modifier (also supported on musl and bionic), which side-steps the issues as much as possible. Co-authored-by: Cristian Rodríguez <cristian@rodriguez.im> [emil: split from larger patch, convert more instances to %m] Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Currently we reset errno prior to calling strto{u,}l(). This is not needed, since a) we don't check errno to see if the function was successful and b) we explicitly propagate the error code by returning it directly to the caller. Reference: #244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Currently we reset errno, shortly to be followed by an fcntl() and gzdopen() calls. Both of those should set errno on failure and preserve it on success. Just leave errno as-is, we shouldn't be changing it in this context. Reference: #244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

The single caller of add_param() does not need the struct param*, so we might as well return the error code directly. As a result we don't manually overwrite errno. Reference: #244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Change the function signature (bool -> int) and directly return the error code. Thus we no longer need to overwrite errno. v2: - return false -> return -ENAMETOOLONG Reference: #244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Currently our wrapper init_module() will happily return success whenever libkmod fails. While such failures are unlikely, our wrapper should also fail. In part so it doesn't mask a potentially deeper problem and in part because the kmod API used, will set errno... Something a normal syscall wouldn't do AFAICT. v2: - remove the respective comment Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

The function does bounds checking, allocation and copying. In the first instance, we manually set errno (to ENOMEM?) on failure. The realloc() call does the same, implicitly. In practice we don't distinguish between the two failures, so we might as well stop manually setting errno and always assume ENOMEM in the caller. Reference: #244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

As per the manual getdelim(3): The buffer is null-terminated and ... Remove the local check and inline the function call. As a nice result, we no longer set the errno and the context of feof() is obvious. Reference: #244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Rework the function signature and return the error code instead of the stripped module. Thus we no longer explicitly set errno. Reference: #244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

In practise none of our code-paths will use a NULL pointer so we might as well enforce that. To stay aligned with the kernel behaviour update our init_module() preload library to return EFAULT... Should we get confused and pass NULL in the future. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

The function returns what is considered constant data and all the callers handle it as such. Add the declaration to make things obvious. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Rework the function signature and return the error code instead of the stripped module. Thus we no longer explicitly set errno. Reference: #244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

Rework the function signature and return the error code instead of the stripped module. Thus we no longer explicitly set errno. v2: - kmod_file_open() - use _cleanup_free_, return errno instead of ENOMEM Reference: #244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #346 Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>

lucasdemarchi · 2025-05-20T02:52:51Z

Applied, thanks.

github-advanced-security bot found potential problems May 10, 2025

View reviewed changes

This was referenced May 10, 2025

Introduce KMOD_STRERROR to ensure usage is safer. #336

Open

Fix errno bugs #343

Closed

stoeckmann suggested changes May 12, 2025

View reviewed changes

libkmod/libkmod-builtin.c Outdated Show resolved Hide resolved

testsuite/init_module.c Show resolved Hide resolved

evelikov force-pushed the strerror-errno branch from 5e55d16 to 274151d Compare May 18, 2025 21:55

lucasdemarchi reviewed May 19, 2025

View reviewed changes

evelikov and others added 14 commits May 19, 2025 19:50

libkmod: fixup kmod_module_get_initstate() error reporting

a395c04

Currently, the function will report the exact same issue twice. Where the second DBG() and the error code returned, should highlight the (potential) stat(2) failure. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

libkmod: return the errno from kmod_elf_strip()

28b494d

Rework the function signature and return the error code instead of the stripped module. Thus we no longer explicitly set errno. Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

libkmod: kmod_file_get_contents() returns constant data

4ede41c

The function returns what is considered constant data and all the callers handle it as such. Add the declaration to make things obvious. Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

libkmod: return the errno from kmod_elf_new()

eacfa49

Rework the function signature and return the error code instead of the stripped module. Thus we no longer explicitly set errno. Reference: kmod-project#244 Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com>

evelikov force-pushed the strerror-errno branch from 274151d to 93570eb Compare May 19, 2025 18:58

lucasdemarchi closed this May 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reduce strerror and errno usage #346

Reduce strerror and errno usage #346

Uh oh!

evelikov commented May 10, 2025

Uh oh!

Check failure

Check failure

evelikov commented May 11, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 18, 2025 •

edited

Loading

Uh oh!

lucasdemarchi May 19, 2025

Uh oh!

evelikov May 19, 2025

Uh oh!

lucasdemarchi May 20, 2025

Uh oh!

lucasdemarchi commented May 20, 2025

Uh oh!

Uh oh!

Reduce strerror and errno usage #346

Reduce strerror and errno usage #346

Uh oh!

Conversation

evelikov commented May 10, 2025

Uh oh!

Check failure

Uh oh!

Uh oh!

Uh oh!

Check failure

Uh oh!

Uh oh!

Uh oh!

evelikov commented May 11, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lucasdemarchi May 19, 2025

Choose a reason for hiding this comment

Uh oh!

evelikov May 19, 2025

Choose a reason for hiding this comment

Uh oh!

lucasdemarchi May 20, 2025

Choose a reason for hiding this comment

Uh oh!

lucasdemarchi commented May 20, 2025

Uh oh!

Uh oh!

codecov bot commented May 18, 2025 •

edited

Loading