[PATCH 0/5] pull: couple clang scan-build fixes

[PATCH 0/5] pull: couple clang scan-build fixes

[Sami Kerola]

Hello,

Here are some minor updates to make static analyzers less noisy.  Whether
these make any difference in real life is questionable.  Well at least there
will be fewer false positive when running scan-build, asan, or such.


----------------------------------------------------------------
The following changes since commit 1bd62f72d8281cdd089fbef43652656dd3c60642:
  libblkid: fix mistake in debug message (2016-04-19 12:45:00 +0200)
are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git scan-build
for you to fetch changes up to 55cf10134a5dd3622361090ea90e7fd501b4b655:
  getopt: fix memory leaks and integer overflows [ASAN & valgrind] (2016-04-19 21:27:54 +0100)
----------------------------------------------------------------

Sami Kerola (5):
  build-sys: test functions does not return void
  libmount: fix memory leak
  setpwnam: fix memory leak
  lib: avoid double free in loopdev.c
  getopt: fix memory leaks and integer overflows [ASAN & valgrind]

 configure.ac           |  2 +-
 lib/loopdev.c          |  1 +
 libmount/src/utils.c   |  4 ++--
 login-utils/setpwnam.c |  1 +
 misc-utils/getopt.c    | 25 ++++++++++++++++++-------
 5 files changed, 23 insertions(+), 10 deletions(-)

-- 
2.8.0

[PATCH 1/5] build-sys: test functions does not return void

[Sami Kerola]

Found using scan-build.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 5a00403..b48a24a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -491,7 +491,7 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
 
 AC_MSG_CHECKING([whether __progname is defined])
 AC_LINK_IFELSE([AC_LANG_PROGRAM([extern char *__progname;],
-	   [if (*__progname == 0) return;])],
+	   [if (*__progname == 0) return 1;])],
 	   AC_DEFINE([HAVE___PROGNAME], [1], [Define if __progname is defined])
 	   AC_MSG_RESULT([yes]),
 	   AC_MSG_RESULT([no]))
-- 
2.8.0

[PATCH 2/5] libmount: fix memory leak

[Sami Kerola]

Found with scan-build.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 libmount/src/utils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libmount/src/utils.c b/libmount/src/utils.c
index 0fcc1d0..8b80609 100644
--- a/libmount/src/utils.c
+++ b/libmount/src/utils.c
@@ -1107,7 +1107,7 @@ char *mnt_get_kernel_cmdline_option(const char *name)
 int mnt_guess_system_root(dev_t devno, struct libmnt_cache *cache, char **path)
 {
 	char buf[PATH_MAX];
-	char *dev = NULL, *spec;
+	char *dev = NULL, *spec = NULL;
 	unsigned int x, y;
 	int allocated = 0;
 
@@ -1169,8 +1169,8 @@ int mnt_guess_system_root(dev_t devno, struct libmnt_cache *cache, char **path)
 		if (dev && !cache)
 			allocated = 1;
 	}
-	free(spec);
 done:
+	free(spec);
 	if (dev) {
 		*path = allocated ? dev : strdup(dev);
 		if (!path)
-- 
2.8.0

[PATCH 3/5] setpwnam: fix memory leak

[Sami Kerola]

Found with scan-build.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/setpwnam.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/login-utils/setpwnam.c b/login-utils/setpwnam.c
index 9f39d01..0616c79 100644
--- a/login-utils/setpwnam.c
+++ b/login-utils/setpwnam.c
@@ -166,6 +166,7 @@ int setpwnam(struct passwd *pwd, const char *prefix)
 		goto fail;
 	/* finally:  success */
 	ulckpwdf();
+	free(linebuf);
 	return 0;
 
  fail:
-- 
2.8.0

[PATCH 4/5] lib: avoid double free in loopdev.c

[Sami Kerola]

Found with scan-build.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 lib/loopdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/loopdev.c b/lib/loopdev.c
index 9ae61da..ca5d2f4 100644
--- a/lib/loopdev.c
+++ b/lib/loopdev.c
@@ -460,6 +460,7 @@ static int loop_scandir(const char *dirname, int **ary, int hasprefix)
 			tmp = realloc(*ary, arylen * sizeof(int));
 			if (!tmp) {
 				free(*ary);
+				ary = NULL;
 				closedir(dir);
 				return -1;
 			}
-- 
2.8.0

[PATCH 5/5] getopt: fix memory leaks and integer overflows [ASAN & valgrind]

[Sami Kerola]

The getopt(1) is short living command, and one could argue ensuring all
allocations are freed at end of execution is waste of time.  There is a
point in that, but making test-suite runs to be less noisy with ASAN is also
nice as it encourages reading the errors when/if they happen.

Reviewed-by: Yuriy M. Kaminskiy <yumkam@gmail.com>
Reviewed-by: Karel Zak <kzak@redhat.com>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/getopt.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/misc-utils/getopt.c b/misc-utils/getopt.c
index 2cff2eb..dc2a976 100644
--- a/misc-utils/getopt.c
+++ b/misc-utils/getopt.c
@@ -86,6 +86,7 @@ struct getopt_control {
 	int long_options_length;	/* length of options array */
 	int long_options_nr;		/* number of used elements in array */
 	unsigned int
+		free_name:1,		/* free up argv[0] after printout */
 		compatible:1,		/* compatibility mode for 'difficult' programs */
 		quiet_errors:1,		/* print errors */
 		quiet_output:1,		/* print output */
@@ -181,7 +182,7 @@ static void print_normalized(const struct getopt_control *ctl, const char *arg)
  * optstr must contain the short options, and longopts the long options.
  * Other settings are found in global variables.
  */
-static int generate_output(const struct getopt_control *ctl, char *argv[], int argc)
+static int generate_output(struct getopt_control *ctl, char *argv[], int argc)
 {
 	int exit_code = EXIT_SUCCESS;	/* Assume everything will be OK */
 	int opt;
@@ -195,8 +196,10 @@ static int generate_output(const struct getopt_control *ctl, char *argv[], int a
 	optind = 0;
 
 	while ((opt =
-		(getopt_long_fp(argc, argv, ctl->optstr, ctl->long_options, &longindex)))
-	       != EOF)
+		(getopt_long_fp
+		 (argc, argv, ctl->optstr,
+		  (const struct option *)ctl->long_options, &longindex)))
+	       != EOF) {
 		if (opt == '?' || opt == ':')
 			exit_code = GETOPT_EXIT_CODE;
 		else if (!ctl->quiet_output) {
@@ -216,13 +219,19 @@ static int generate_output(const struct getopt_control *ctl, char *argv[], int a
 					print_normalized(ctl, optarg ? optarg : "");
 			}
 		}
-
+	}
 	if (!ctl->quiet_output) {
 		printf(" --");
 		while (optind < argc)
 			print_normalized(ctl, argv[optind++]);
 		printf("\n");
 	}
+	for (longindex = 0; longindex < ctl->long_options_nr; longindex++)
+		free((char *)ctl->long_options[longindex].name);
+	free(ctl->long_options);
+	free(ctl->optstr);
+	if (ctl->free_name)
+		free(argv[0]);
 	return exit_code;
 }
 
@@ -373,9 +382,6 @@ int main(int argc, char *argv[])
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	add_longopt(&ctl, NULL, 0);	/* init */
-	getopt_long_fp = getopt_long;
-
 	if (getenv("GETOPT_COMPATIBLE"))
 		ctl.compatible = 1;
 
@@ -391,6 +397,9 @@ int main(int argc, char *argv[])
 			parse_error(_("missing optstring argument"));
 	}
 
+	add_longopt(&ctl, NULL, 0);	/* init */
+	getopt_long_fp = getopt_long;
+
 	if (argv[1][0] != '-' || ctl.compatible) {
 		ctl.quote = 0;
 		ctl.optstr = xmalloc(strlen(argv[1]) + 1);
@@ -417,6 +426,7 @@ int main(int argc, char *argv[])
 		case 'n':
 			free(name);
 			name = xstrdup(optarg);
+			ctl.free_name = 1;
 			break;
 		case 'q':
 			ctl.quiet_errors = 1;
@@ -428,6 +438,7 @@ int main(int argc, char *argv[])
 			ctl.shell = shell_type(optarg);
 			break;
 		case 'T':
+			free(ctl.long_options);
 			return TEST_EXIT_CODE;
 		case 'u':
 			ctl.quote = 0;
-- 
2.8.0

Re: [PATCH 4/5] lib: avoid double free in loopdev.c

[Yuriy M. Kaminskiy]

Sami Kerola <kerolasa@iki.fi> writes:

> Found with scan-build.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  lib/loopdev.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/loopdev.c b/lib/loopdev.c
> index 9ae61da..ca5d2f4 100644
> --- a/lib/loopdev.c
> +++ b/lib/loopdev.c
> @@ -460,6 +460,7 @@ static int loop_scandir(const char *dirname, int **ary, int hasprefix)
>  			tmp = realloc(*ary, arylen * sizeof(int));
>  			if (!tmp) {
>  				free(*ary);
> +				ary = NULL;

This cannot have any effect. Probably you wanted
                                *ary = NULL;
                                
>  				closedir(dir);
>  				return -1;
>  			}

Re: [PATCH 4/5] lib: avoid double free in loopdev.c

[Sami Kerola]

On 19 April 2016 at 23:52, Yuriy M. Kaminskiy <yumkam@gmail.com> wrote:
> Sami Kerola <kerolasa@iki.fi> writes:
>
>> Found with scan-build.
>>
>> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
>> ---
>>  lib/loopdev.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/loopdev.c b/lib/loopdev.c
>> index 9ae61da..ca5d2f4 100644
>> --- a/lib/loopdev.c
>> +++ b/lib/loopdev.c
>> @@ -460,6 +460,7 @@ static int loop_scandir(const char *dirname, int **ary, int hasprefix)
>>                       tmp = realloc(*ary, arylen * sizeof(int));
>>                       if (!tmp) {
>>                               free(*ary);
>> +                             ary = NULL;
>
> This cannot have any effect. Probably you wanted
>                                 *ary = NULL;

Thank you for review Yuriy. Fixed in

https://github.com/kerolasa/lelux-utiliteetit/commit/34f6177a24cf602d0720b82f2064e6830ba34ffb

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 0/5] pull: couple clang scan-build fixes

[Karel Zak]

On Tue, Apr 19, 2016 at 09:35:40PM +0100, Sami Kerola wrote:
>   build-sys: test functions does not return void
>   libmount: fix memory leak
>   setpwnam: fix memory leak
>   lib: avoid double free in loopdev.c
>   getopt: fix memory leaks and integer overflows [ASAN & valgrind]

 Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 5/5] getopt: fix memory leaks and integer overflows [ASAN & valgrind]

[Karel Zak]

On Tue, Apr 19, 2016 at 09:35:45PM +0100, Sami Kerola wrote:
> The getopt(1) is short living command, and one could argue ensuring all
> allocations are freed at end of execution is waste of time.  There is a
> point in that, but making test-suite runs to be less noisy with ASAN is also
> nice as it encourages reading the errors when/if they happen.
> 
> Reviewed-by: Yuriy M. Kaminskiy <yumkam@gmail.com>
> Reviewed-by: Karel Zak <kzak@redhat.com>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  misc-utils/getopt.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/misc-utils/getopt.c b/misc-utils/getopt.c
> index 2cff2eb..dc2a976 100644
> --- a/misc-utils/getopt.c
> +++ b/misc-utils/getopt.c
> @@ -86,6 +86,7 @@ struct getopt_control {
>  	int long_options_length;	/* length of options array */
>  	int long_options_nr;		/* number of used elements in array */
>  	unsigned int
> +		free_name:1,		/* free up argv[0] after printout */

 I did a change to the code, it seems more elegant to keep pointer to
 the 'name' in the control struct than maintain free_name flag and
 call free(argv[0]).

    https://github.com/karelzak/util-linux/commit/e402d6d3b149c34faa5abd5a0cef8284b4ae1af3


  Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com