usr/gen_init_cpio.c: remove unnecessary "if"

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 <[email protected]>

---
 usr/gen_init_cpio.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

usr/gen_init_cpio.c +1 -1

index af8c925..afebe09 100644
@@ -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);

[resend, busted mailer]

Quoted 26 lines · Cong Ding

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 <[email protected]>

---
 usr/gen_init_cpio.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

usr/gen_init_cpio.c +1 -1

index af8c925..afebe09 100644
@@ -372,7 +372,7 @@ static int cpio_mkfile(const char *name, const char
 *location,
        }
        ino++;
        rc = 0;
-
+
 error:
        if (filebuf) free(filebuf);

Quoted 1 line

        if (file >= 0) close(file);

This chunk is fine.

usr/gen_init_cpio.c static int cpio_mkfile_line(const char *line) · line 458

Quoted 9 lines

usr/gen_init_cpio.c +1 -1

@@ -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

Quoted 58 lines · Kees Cook

[resend, busted mailer]

Quoted 26 lines · Cong Ding

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 <[email protected]>

---
 usr/gen_init_cpio.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

usr/gen_init_cpio.c +1 -1

index af8c925..afebe09 100644
@@ -372,7 +372,7 @@ static int cpio_mkfile(const char *name, const char
 *location,
        }
        ino++;
        rc = 0;
-
+
 error:
        if (filebuf) free(filebuf);

Quoted 1 line

        if (file >= 0) close(file);

This chunk is fine.

usr/gen_init_cpio.c static int cpio_mkfile_line(const char *line) · line 458

Quoted 9 lines

usr/gen_init_cpio.c +1 -1

@@ -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;

Quoted 5 lines

}
rc = cpio_mkfile(name_arg, ...);
fail:
free(dname);
return rc;

sorry my patch is wrong (I missed the "else" branch). but I don't think it is necessary to use another variable name_arg. so I think the correct version would be as following. if you think it is necessary and agree the change, I will send a version 2.

usr/gen_init_cpio.c +2 -2

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

or you want something like this? I have added variable name_arg, changed the dname_len to name_len, changed fail to out, moved free(dname) before "out". (btw, if we do like this, what do you suggest to use in the commit message?)

 usr/gen_init_cpio.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

usr/gen_init_cpio.c +3 -2

index af8c925..dff6a1e 100644
@@ -409,19 +409,20 @@ static int cpio_mkfile_line(const char *line)
 {
 	char name[PATH_MAX + 1];
 	char *dname = NULL; /* malloc'ed buffer for hard links */
+	char *name_arg;
 	char location[PATH_MAX + 1];
 	unsigned int mode;
 	int uid;
 	int gid;
 	int nlinks = 1;
-	int end = 0, dname_len = 0;
+	int end = 0, name_len = 0;
 	int rc = -1;
 
 	if (5 > sscanf(line, "%" str(PATH_MAX) "s %" str(PATH_MAX)
 				"s %o %d %d %n",
 				name, location, &mode, &uid, &gid, &end)) {
 		fprintf(stderr, "Unrecognized file format '%s'", line);
-		goto fail;
+		goto out;
 	}
 	if (end && isgraph(line[end])) {
 		int len;

usr/gen_init_cpio.c +10 -8

+			goto out;
 		}
 
-		dname_len = strlen(name) + 1;
-		memcpy(dname, name, dname_len);
+		name_arg = dname;
+		name_len = strlen(name) + 1;
+		memcpy(name_arg, name, name_len);
 
 		do {
 			nend = 0;

@@ -442,18 +444,18 @@ static int cpio_mkfile_line(const char *line)
 					name, &nend) < 1)
 				break;
 			len = strlen(name) + 1;
-			memcpy(dname + dname_len, name, len);
-			dname_len += len;
+			memcpy(name_arg + name_len, name, len);
+			name_len += len;
 			nlinks++;
 			end += nend;
 		} while (isgraph(line[end]));
 	} else {
-		dname = name;
+		name_arg = name;
 	}
-	rc = cpio_mkfile(dname, cpio_replace_env(location),
+	rc = cpio_mkfile(name_arg, cpio_replace_env(location),
 	                 mode, uid, gid, nlinks);
- fail:
-	if (dname_len) free(dname);
+	if (!dname) free(dname);
+ out:
 	return rc;
 }
 

Signature · 8 lines

1.7.10.4




 
-- 
1.7.10.4

Quoted 8 lines

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

-Kees

--
Kees Cook
Chrome OS Security