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

[Kees Cook]

[resend, busted mailer]

On Tue, Dec 18, 2012 at 1:26 PM, Cong Ding <dinggnu@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/