Re: patch: properly check NULL return values

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

Re: patch: properly check NULL return values

Tobias Stoeckmann-5
Hi,

turns out that there are a few more bad savestr calls even inside of pch.c.
Some of the code pathes are quite obvious that a NULL return value would
lead to a null pointer dereference, others would last longer before
dereferencing.

Carefully reviewing the savestr calls, the rule of thumb seems to be:
If savestr is used, immediately check out_of_mem, otherwise use xstrdup.

Theo pointed out that this one is very ugly (indeed):

+ if (!s)
+ s = "Oops";

Yet it's current behavior of savestr.  I want to remove that in another
commit.  So for now, xstrdup behaves like savestr, calling fatal in case
of out of memory situation -- regardless of "plan a" or "plan b".


Tobias

Index: patch.c
===================================================================
RCS file: /cvs/src/usr.bin/patch/patch.c,v
retrieving revision 1.51
diff -u -p -r1.51 patch.c
--- patch.c 26 Nov 2013 13:19:07 -0000 1.51
+++ patch.c 26 Nov 2014 11:46:20 -0000
@@ -213,7 +213,7 @@ main(int argc, char *argv[])
  warn_on_invalid_line = true;
 
  if (outname == NULL)
- outname = savestr(filearg[0]);
+ outname = xstrdup(filearg[0]);
 
  /* for ed script just up and do it and exit */
  if (diff_type == ED_DIFF) {
@@ -491,10 +491,10 @@ get_some_switches(void)
  /* FALLTHROUGH */
  case 'z':
  /* must directly follow 'b' case for backwards compat */
- simple_backup_suffix = savestr(optarg);
+ simple_backup_suffix = xstrdup(optarg);
  break;
  case 'B':
- origprae = savestr(optarg);
+ origprae = xstrdup(optarg);
  break;
  case 'c':
  diff_type = CONTEXT_DIFF;
@@ -532,7 +532,7 @@ get_some_switches(void)
  case 'i':
  if (++filec == MAXFILEC)
  fatal("too many file arguments\n");
- filearg[filec] = savestr(optarg);
+ filearg[filec] = xstrdup(optarg);
  break;
  case 'l':
  canonicalize = true;
@@ -544,7 +544,7 @@ get_some_switches(void)
  noreverse = true;
  break;
  case 'o':
- outname = savestr(optarg);
+ outname = xstrdup(optarg);
  break;
  case 'p':
  strippath = atoi(optarg);
@@ -588,12 +588,12 @@ get_some_switches(void)
  Argv += optind;
 
  if (Argc > 0) {
- filearg[0] = savestr(*Argv++);
+ filearg[0] = xstrdup(*Argv++);
  Argc--;
  while (Argc > 0) {
  if (++filec == MAXFILEC)
  fatal("too many file arguments\n");
- filearg[filec] = savestr(*Argv++);
+ filearg[filec] = xstrdup(*Argv++);
  Argc--;
  }
  }
Index: pch.c
===================================================================
RCS file: /cvs/src/usr.bin/patch/pch.c,v
retrieving revision 1.46
diff -u -p -r1.46 pch.c
--- pch.c 26 Nov 2014 10:11:21 -0000 1.46
+++ pch.c 26 Nov 2014 11:46:21 -0000
@@ -215,14 +215,14 @@ there_is_another_patch(void)
  while (filearg[0] == NULL) {
  if (force || batch) {
  say("No file to patch.  Skipping...\n");
- filearg[0] = savestr(bestguess);
+ filearg[0] = xstrdup(bestguess);
  skip_rest_of_patch = true;
  return true;
  }
  ask("File to patch: ");
  if (*buf != '\n') {
  free(bestguess);
- bestguess = savestr(buf);
+ bestguess = xstrdup(buf);
  filearg[0] = fetchname(buf, &exists, 0);
  }
  if (!exists) {
@@ -310,7 +310,7 @@ intuit_diff_type(void)
  else if (strnEQ(s, "Prereq:", 7)) {
  for (t = s + 7; isspace((unsigned char)*t); t++)
  ;
- revision = savestr(t);
+ revision = xstrdup(t);
  for (t = revision;
     *t && !isspace((unsigned char)*t); t++)
  ;
@@ -389,7 +389,7 @@ scan_exit:
  free(bestguess);
  bestguess = NULL;
  if (filearg[0] != NULL)
- bestguess = savestr(filearg[0]);
+ bestguess = xstrdup(filearg[0]);
  else if (!ok_to_create_file) {
  /*
  * We don't want to create a new file but we need a
@@ -1473,7 +1473,7 @@ posix_name(const struct file_name *names
  path = names[NEW_FILE].path;
  }
 
- return path ? savestr(path) : NULL;
+ return path ? xstrdup(path) : NULL;
 }
 
 static char *
@@ -1538,7 +1538,7 @@ best_name(const struct file_name *names,
     names[NEW_FILE].path != NULL)
  best = names[NEW_FILE].path;
  }
- return best ? savestr(best) : NULL;
+ return best ? xstrdup(best) : NULL;
 }
 
 static size_t
Index: util.c
===================================================================
RCS file: /cvs/src/usr.bin/patch/util.c,v
retrieving revision 1.37
diff -u -p -r1.37 util.c
--- util.c 22 Nov 2014 15:49:28 -0000 1.37
+++ util.c 26 Nov 2014 11:46:21 -0000
@@ -192,6 +192,22 @@ savestr(const char *s)
 }
 
 /*
+ * Allocate a unique area for a string.  Call fatal if out of memory.
+ */
+char *
+xstrdup(const char *s)
+{
+ char *rv;
+
+ if (!s)
+ s = "Oops";
+ rv = strdup(s);
+ if (rv == NULL)
+ fatal("out of memory\n");
+ return rv;
+}
+
+/*
  * Vanilla terminal output (buffered).
  */
 void
Index: util.h
===================================================================
RCS file: /cvs/src/usr.bin/patch/util.h,v
retrieving revision 1.15
diff -u -p -r1.15 util.h
--- util.h 20 Jun 2005 07:14:06 -0000 1.15
+++ util.h 26 Nov 2014 11:46:21 -0000
@@ -40,6 +40,7 @@ void pfatal(const char *, ...)
 void ask(const char *, ...)
     __attribute__((__format__(__printf__, 1, 2)));
 char *savestr(const char *);
+char *xstrdup(const char *);
 void set_signals(int);
 void ignore_signals(void);
 void makedirs(const char *, bool);