linux-kernel - Re: [PATCH] usr/gen_init_cpio.c: remove unnecessary "if"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5jKfrBikf1ND6Oeadwnr6Zh4wG2raF8BCNOxN2TJBwTedw@mail.gmail.com>
Date:	Tue, 18 Dec 2012 14:03:08 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Cong Ding <dinggnu@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Jiri Kosina <jkosina@...e.cz>, Jesper Juhl <jj@...esealer.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] usr/gen_init_cpio.c: remove unnecessary "if"

[resend, busted mailer]

On Tue, Dec 18, 2012 at 1:26 PM, Cong Ding <dinggnu@...il.com> wrote:
>
> usr/gen_init_cpio.c: remove unnecessary "if"
>
> if it goes to fail, dname isn't allocated; otherwise, it is allocated
> successfully. so we just free memory when it doesn't fail.
>
> this patch also fix a trival trailing space warning by checkpatch
>
> Signed-off-by: Cong Ding <dinggnu@...il.com>
> ---
>  usr/gen_init_cpio.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
> index af8c925..afebe09 100644
> --- a/usr/gen_init_cpio.c
> +++ b/usr/gen_init_cpio.c
> @@ -372,7 +372,7 @@ static int cpio_mkfile(const char *name, const char
> *location,
>         }
>         ino++;
>         rc = 0;
> -
> +
>  error:
>         if (filebuf) free(filebuf);
>         if (file >= 0) close(file);


This chunk is fine.

> @@ -452,8 +452,8 @@ static int cpio_mkfile_line(const char *line)
>         }
>         rc = cpio_mkfile(dname, cpio_replace_env(location),
>                          mode, uid, gid, nlinks);
> +       free(dname);
>   fail:
> -       if (dname_len) free(dname);
>         return rc;
>  }

While technically correct, the dname_len check is there to avoid the case of
trying to free "name" if a "goto fail" is ever added to the code. So, it's
defensive given the mixing of dname being either allocated or static. I
think it might be cleaner to use a new char * (maybe "name_arg", instead of
re-using dname), and then dname can always be freed before the function
returns:

  char * name_arg;
  ...
  if (end && ...) {
      ...
      name_arg = dname;
  } else {
      name_arg = name;
  }
  rc = cpio_mkfile(name_arg, ...);
fail:
  free(dname);
  return rc;

And I'd probably rename "fail" to "out".

-Kees

--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/