libkmod: fix uninitialized variable usage warnings

PATCH In review Eugene Syromiatnikov 2 participants 4 messages Nov 6, 2017 – Nov 7, 2017

Share & tools

Eugene Syromiatnikov

author

2017-11-06 21:56 UTC #0 lore raw

PATCH 1/1 libkmod: fix uninitialized variable usage warnings

There are two places where _cleanup_free_ variables are not initialised by the time function exits that have been caught by gcc:

	In file included from libkmod/libkmod.c:35:0:
	libkmod/libkmod.c: In function 'kmod_lookup_alias_is_builtin':
	./shared/util.h:73:13: warning: 'line' may be used uninitialized in this function [-Wmaybe-uninitialized]
		 free(*(void**) p);
		     ^
	libkmod/libkmod.c:551:23: note: 'line' was declared here
	  _cleanup_free_ char *line;
			       ^
	In file included from libkmod/libkmod-module.c:42:0:
	libkmod/libkmod-module.c: In function 'kmod_module_probe_insert_module':
	./shared/util.h:73:13: warning: 'cmd' may be used uninitialized in this function [-Wmaybe-uninitialized]
		 free(*(void**) p);
		     ^
	libkmod/libkmod-module.c:996:23: note: 'cmd' was declared here
	  _cleanup_free_ char *cmd;
                       ^

This patch initializes them to NULL so free become no-op in these cases.

---
 libkmod/libkmod-module.c | 2 +-
 libkmod/libkmod.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

libkmod/libkmod-module.c +1 -1

index 0a3ef11..6f23c1a 100644
@@ -995,7 +995,7 @@ static int module_do_install_commands(struct kmod_module *mod,
 {
 	const char *command = kmod_module_get_install_commands(mod);
 	char *p;
-	_cleanup_free_ char *cmd;
+	_cleanup_free_ char *cmd = NULL;
 	int err;
 	size_t cmdlen, options_len, varlen;

libkmod/libkmod.c +1 -1

index 69fe431..964772d 100644
@@ -556,7 +556,7 @@ finish:
 
 bool kmod_lookup_alias_is_builtin(struct kmod_ctx *ctx, const char *name)
 {
-	_cleanup_free_ char *line;
+	_cleanup_free_ char *line = NULL;
 
 	line = lookup_builtin_file(ctx, name);

Signature · 1 line

2.1.4

Lucas De Marchi

2017-11-07 03:24 UTC #1 lore raw

Quoted 44 lines · Eugene Syromiatnikov

There are two places where _cleanup_free_ variables are not initialised by the time function exits that have been caught by gcc:
In file included from libkmod/libkmod.c:35:0:
libkmod/libkmod.c: In function 'kmod_lookup_alias_is_builtin':
./shared/util.h:73:13: warning: 'line' may be used uninitialized in this function [-Wmaybe-uninitialized]
free(*(void**) p);
^
libkmod/libkmod.c:551:23: note: 'line' was declared here
_cleanup_free_ char *line;
^
In file included from libkmod/libkmod-module.c:42:0:
libkmod/libkmod-module.c: In function 'kmod_module_probe_insert_module':
./shared/util.h:73:13: warning: 'cmd' may be used uninitialized in this function [-Wmaybe-uninitialized]
free(*(void**) p);
^
libkmod/libkmod-module.c:996:23: note: 'cmd' was declared here
_cleanup_free_ char *cmd;
^
This patch initializes them to NULL so free become no-op in these cases.
---
 libkmod/libkmod-module.c | 2 +-
 libkmod/libkmod.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
libkmod/libkmod-module.c +1 -1
index 0a3ef11..6f23c1a 100644
@@ -995,7 +995,7 @@ static int module_do_install_commands(struct kmod_module *mod,
 {
        const char *command = kmod_module_get_install_commands(mod);
        char *p;
-       _cleanup_free_ char *cmd;
+       _cleanup_free_ char *cmd = NULL;
        int err;
        size_t cmdlen, options_len, varlen;
 
libkmod/libkmod.c +0 -0
index 69fe431..964772d 100644

libkmod/libkmod.c finish: · line 559

Quoted 5 lines

libkmod/libkmod.c +1 -1

@@ -556,7 +556,7 @@ finish:
 
 bool kmod_lookup_alias_is_builtin(struct kmod_ctx *ctx, const char *name)
 {
-       _cleanup_free_ char *line;
+       _cleanup_free_ char *line = NULL;

This seems to be a bogus warning. See the line just below... there's no way to exit this function without first assigning 'line'.

Lucas De Marchi

Eugene Syromiatnikov

author

2017-11-07 11:59 UTC #2 lore raw

Quoted 46 lines · Lucas De Marchi

Quoted 45 lines · Eugene Syromiatnikov
There are two places where _cleanup_free_ variables are not initialised by the time function exits that have been caught by gcc:
In file included from libkmod/libkmod.c:35:0:
libkmod/libkmod.c: In function 'kmod_lookup_alias_is_builtin':
./shared/util.h:73:13: warning: 'line' may be used uninitialized in this function [-Wmaybe-uninitialized]
free(*(void**) p);
^
libkmod/libkmod.c:551:23: note: 'line' was declared here
_cleanup_free_ char *line;
^
In file included from libkmod/libkmod-module.c:42:0:
libkmod/libkmod-module.c: In function 'kmod_module_probe_insert_module':
./shared/util.h:73:13: warning: 'cmd' may be used uninitialized in this function [-Wmaybe-uninitialized]
free(*(void**) p);
^
libkmod/libkmod-module.c:996:23: note: 'cmd' was declared here
_cleanup_free_ char *cmd;
^
This patch initializes them to NULL so free become no-op in these cases.
---
 libkmod/libkmod-module.c | 2 +-
 libkmod/libkmod.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
libkmod/libkmod-module.c +1 -1
index 0a3ef11..6f23c1a 100644
@@ -995,7 +995,7 @@ static int module_do_install_commands(struct kmod_module *mod,
 {
        const char *command = kmod_module_get_install_commands(mod);
        char *p;
-       _cleanup_free_ char *cmd;
+       _cleanup_free_ char *cmd = NULL;
        int err;
        size_t cmdlen, options_len, varlen;
 
libkmod/libkmod.c +0 -0
index 69fe431..964772d 100644
@@ -556,7 +556,7 @@ finish:
 

Quoted 7 lines

libkmod/libkmod.c finish: · line 559
Quoted 4 lines
libkmod/libkmod.c +1 -1
 bool kmod_lookup_alias_is_builtin(struct kmod_ctx *ctx, const char *name)
 {
-       _cleanup_free_ char *line;
+       _cleanup_free_ char *line = NULL;
This seems to be a bogus warning. See the line just below... there's no way to exit this function without first assigning 'line'.

In this instance, I initially thought that gcc is smart and derives "uninitialised variable" from the lookup_builtin_file call, but nope, it was happy only after line was initialised on declaration. On the other hand, documentation[1] mentions that cleanup function is also called during stack unwinding in case -fexceptions is enabled, and RHEL's RPM package indeed has this flag, at least, so this GCC's complaint still seems relevant.

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html

Quoted 1 line

Lucas De Marchi

Lucas De Marchi

2017-11-07 16:45 UTC #3 lore raw

Quoted 55 lines · Eugene Syromiatnikov

Quoted 46 lines · Lucas De Marchi
Quoted 45 lines · Eugene Syromiatnikov
There are two places where _cleanup_free_ variables are not initialised by the time function exits that have been caught by gcc:
In file included from libkmod/libkmod.c:35:0:
libkmod/libkmod.c: In function 'kmod_lookup_alias_is_builtin':
./shared/util.h:73:13: warning: 'line' may be used uninitialized in this function [-Wmaybe-uninitialized]
free(*(void**) p);
^
libkmod/libkmod.c:551:23: note: 'line' was declared here
_cleanup_free_ char *line;
^
In file included from libkmod/libkmod-module.c:42:0:
libkmod/libkmod-module.c: In function 'kmod_module_probe_insert_module':
./shared/util.h:73:13: warning: 'cmd' may be used uninitialized in this function [-Wmaybe-uninitialized]
free(*(void**) p);
^
libkmod/libkmod-module.c:996:23: note: 'cmd' was declared here
_cleanup_free_ char *cmd;
^
This patch initializes them to NULL so free become no-op in these cases.
---
 libkmod/libkmod-module.c | 2 +-
 libkmod/libkmod.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
libkmod/libkmod-module.c +1 -1
index 0a3ef11..6f23c1a 100644
@@ -995,7 +995,7 @@ static int module_do_install_commands(struct kmod_module *mod,
 {
        const char *command = kmod_module_get_install_commands(mod);
        char *p;
-       _cleanup_free_ char *cmd;
+       _cleanup_free_ char *cmd = NULL;
        int err;
        size_t cmdlen, options_len, varlen;
 
libkmod/libkmod.c +0 -0
index 69fe431..964772d 100644
@@ -556,7 +556,7 @@ finish:
 
Quoted 7 lines
libkmod/libkmod.c finish: · line 559
Quoted 4 lines
libkmod/libkmod.c +1 -1
 bool kmod_lookup_alias_is_builtin(struct kmod_ctx *ctx, const char *name)
 {
-       _cleanup_free_ char *line;
+       _cleanup_free_ char *line = NULL;
This seems to be a bogus warning. See the line just below... there's no way to exit this function without first assigning 'line'.

Quoted 7 lines

In this instance, I initially thought that gcc is smart and derives "uninitialised variable" from the lookup_builtin_file call, but nope, it was happy only after line was initialised on declaration. On the other hand, documentation[1] mentions that cleanup function is also called during stack unwinding in case -fexceptions is enabled, and RHEL's RPM package indeed has this flag, at least, so this GCC's complaint still seems relevant.

This is likely to trigger "double assignment" warnings on other compilers. A good question would be.... why is -fexceptions being used in a C-only library? If it was a binary or a library that links to a C++-library, I would understand, but in
this case it seems nonsense. /me lost

An acceptable solution would be to just merge the 2 lines so we initialize line on
declaration.

I added -fexceptions here to reproduce the issue, but it didn't reproduce using gcc 7.2.1
on Fedora 27. I also remember not having the issue with gcc 6. So I still think it's
a compiler bug, although I don't mind if the solution proposed above is applied (but mention
on commit message the exact compiler version).

Lucas De Marchi