spamd-setup cleanup

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

spamd-setup cleanup

Kjell-5
Ugh. This got out of control.
All I wanted to do was to clean up the memory handling in
spamd-setup. Of course, that lead to the size_t vs. int type
confusion, which led to array index bugs, which led to...


Well, you can read it. Please have a careful look at the following,
Of course, I went *way* overboard on the paranoia, for no good reason.
There's also no real reason for the pretty realloc failure handling,
except to future proof it, in case someone wants to remove the
errx calls later.

On the upside, this is now much more lint-friendly.

Index: spamd-setup.c
===================================================================
RCS file: /usr/local/cvsweb/openbsd/src/libexec/spamd-setup/spamd-setup.c,v
retrieving revision 1.26
diff -u -r1.26 spamd-setup.c
--- spamd-setup.c 26 Mar 2006 23:54:00 -0000 1.26
+++ spamd-setup.c 28 Mar 2006 08:05:10 -0000
@@ -64,6 +64,7 @@
  int count;
 };
 
+void *  resalloc(void *, size_t, size_t);
 u_int32_t  imask(u_int8_t);
 u_int8_t  maxblock(u_int32_t, u_int8_t);
 u_int8_t  maxdiff(u_int32_t, u_int32_t);
@@ -76,9 +77,9 @@
 int  open_file(char *, char *);
 char *fix_quoted_colons(char *);
 void  do_message(FILE *, char *);
-struct bl *add_blacklist(struct bl *, int *, int *, gzFile, int);
+struct bl *add_blacklist(struct bl *, size_t *, size_t *, gzFile, int);
 int  cmpbl(const void *, const void *);
-struct cidr **collapse_blacklist(struct bl *, int);
+struct cidr **collapse_blacklist(struct bl *, size_t);
 int  configure_spamd(u_short, char *, char *, struct cidr **);
 int  configure_pf(struct cidr **);
 int  getlist(char **, char *, struct blacklist *, struct blacklist *);
@@ -86,6 +87,23 @@
 int  debug;
 int  dryrun;
 
+/* sized realloc */
+void *
+resalloc(void *ptr, size_t num, size_t size)
+{
+ void *nptr;
+
+ if (num && size && SIZE_T_MAX / num < size)
+ goto fail;
+ if ((nptr = realloc(ptr, num * size)) == NULL)
+ goto fail;
+
+ return (nptr);
+fail:
+ errno = ENOMEM;
+ return (NULL);
+}
+
 u_int32_t
 imask(u_int8_t b)
 {
@@ -132,10 +150,9 @@
 struct cidr *
 range2cidrlist(u_int32_t start, u_int32_t end)
 {
- struct cidr *list = NULL;
- size_t cs = 0, cu = 0;
+ struct cidr *list = NULL, *tmp;
+ size_t cs = 0, cu = 0, ncs;
  u_int8_t maxsize, diff;
- struct cidr *tmp;
 
  while (end >= start) {
  maxsize = maxblock(start, 32);
@@ -143,11 +160,12 @@
 
  maxsize = MAX(maxsize, diff);
  if (cs == cu) {
- tmp = realloc(list, (cs + 32) * sizeof(struct cidr));
+ ncs = cs + 32;
+ tmp = resalloc(list, ncs, sizeof(struct cidr));
  if (tmp == NULL)
- errx(1, "malloc failed");
+ err(1, "realloc");
  list = tmp;
- cs += 32;
+ cs = ncs;
  }
  list[cu].addr = start;
  list[cu].bits = maxsize;
@@ -293,23 +311,23 @@
 {
  char *url;
  char **ap, **argv;
- int len, i, oerrno;
+ int i, oerrno;
+ size_t len;
 
  if ((method == NULL) || (strcmp(method, "file") == 0))
  return (open(file, O_RDONLY));
  if ((strcmp(method, "http") == 0) ||
     strcmp(method, "ftp") == 0) {
- asprintf(&url, "%s://%s", method, file);
- if (url == NULL)
+ if (asprintf(&url, "%s://%s", method, file) == -1)
  return (-1);
  i = fileget(url);
  free(url);
  return (i);
  } else if (strcmp(method, "exec") == 0) {
- len = strlen(file);
- argv = malloc(len * sizeof(char *));
- if (argv == NULL)
- errx(1, "malloc failed");
+ if ((len = strlen(file)) == 0)
+ return (-1);
+ if ((argv = calloc(len, sizeof(char *))) == NULL)
+ err(1, "calloc");
  for (ap = argv; ap < &argv[len - 1] &&
     (*ap = strsep(&file, " \t")) != NULL;) {
  if (**ap != '\0')
@@ -341,13 +359,17 @@
 char *
 fix_quoted_colons(char *buf)
 {
- int nbs = 0, i = 0, j = 0, in = 0;
+ int in = 0;
+ size_t bs = 0, nbs, i, j = 0;
  char *newbuf, last, *tmp;
 
- nbs = strlen(buf) + 128;
- newbuf = malloc(nbs);
- if (newbuf == NULL)
- return (NULL);
+ bs = strlen(buf);
+ if (bs > SIZE_T_MAX - 128)
+ errx(1, "overflow");
+ nbs = bs + 128;
+ if ((newbuf = malloc(nbs)) == NULL)
+ err(1, "malloc");
+ bs = nbs;
  last = '\0';
  for (i = 0; i < strlen(buf); i++) {
  switch (buf[i]) {
@@ -366,12 +388,14 @@
  default:
  newbuf[j++] = buf[i];
  }
- if (j == nbs) {
- nbs += 128;
- tmp = realloc(newbuf, nbs);
- if (tmp == NULL)
- errx(1, "malloc failed");
+ if (j == bs) {
+ if (bs > SIZE_T_MAX - 128)
+ errx(1, "overflow");
+ nbs = bs + 128;
+ if ((tmp = realloc(newbuf, nbs)) == NULL)
+ err(1, "realloc");
  newbuf = tmp;
+ bs = nbs;
  }
  }
  free(buf);
@@ -382,17 +406,19 @@
 void
 do_message(FILE *sdc, char *msg)
 {
- int i, n, bu = 0, bs = 0, len;
+ ssize_t n;
+ size_t i, bu = 0, bs = 0, len, nbs;
  char *buf = NULL, last, *tmp;
  int fd;
 
  len = strlen(msg);
  if (msg[0] == '"' && msg[len - 1] == '"') {
+ if (len < 2)
+ errx(1, "message too short");
  /* quoted msg, escape newlines and send it out */
  msg[len - 1] = '\0';
  buf = msg + 1;
  bu = len - 2;
- goto sendit;
  } else {
  /*
  * message isn't quoted - try to open a local
@@ -403,24 +429,25 @@
  err(1, "Can't open message from %s", msg);
  for (;;) {
  if (bu == bs) {
- tmp = realloc(buf, bs + 8192);
- if (tmp == NULL)
- errx(1, "malloc failed");
- bs += 8192;
+ if (bs > SIZE_T_MAX - 8192)
+ errx(1, "Message too large");
+ nbs = bs + 8192;
+ if ((tmp = realloc(buf, nbs)) == NULL)
+ err(1, "realloc");
  buf = tmp;
+ bs = nbs;
  }
 
  n = read(fd, buf + bu, bs - bu);
  if (n == 0) {
- goto sendit;
+ buf[bu] = '\0';
+ break;
  } else if (n == -1) {
  err(1, "Can't read from %s", msg);
  } else
  bu += n;
  }
- buf[bu]='\0';
  }
- sendit:
  fprintf(sdc, ";\"");
  last = '\0';
  for (i = 0; i < bu; i++) {
@@ -439,7 +466,7 @@
  break;
  case '"':
  fputc('\\', sdc);
- /* fall through */
+ /* FALLTHROUGH */
  default:
  fputc(buf[i], sdc);
  last = '\0';
@@ -452,27 +479,32 @@
 
 /* retrieve a list from fd. add to blacklist bl */
 struct bl *
-add_blacklist(struct bl *bl, int *blc, int *bls, gzFile gzf, int white)
+add_blacklist(struct bl *bl, size_t *blc, size_t *bls, gzFile gzf, int white)
 {
- int i, n, start, bu = 0, bs = 0, serrno = 0;
+ int n, serrno = 0;
+ size_t i, start, bu = 0, bs = 0, nbs;
  char *buf = NULL, *tmp;
  struct bl *blt;
 
  for (;;) {
  /* read in gzf, then parse */
  if (bu == bs) {
- tmp = realloc(buf, bs + 8192 + 1);
- if (tmp == NULL) {
+ if (bs > SIZE_T_MAX - 8192)
+ errx(1, "List too large");
+ nbs = bs + 8192;
+ if ((tmp = realloc(buf, nbs)) == NULL) {
+ serrno = errno;
  free(buf);
  buf = NULL;
  bs = 0;
- serrno = errno;
  goto bldone;
  }
- bs += 8192;
+ bs = nbs;
  buf = tmp;
  }
 
+ if ((bs - bu) > UINT_MAX)
+ errx(1, "gz offset too large");
  n = gzread(gzf, buf + bu, bs - bu);
  if (n == 0)
  goto parse;
@@ -486,13 +518,12 @@
  start = 0;
  for (i = 0; i <= bu; i++) {
  if (*blc == *bls) {
- *bls += 1024;
- blt = realloc(bl, *bls * sizeof(struct bl));
- if (blt == NULL) {
- *bls -= 1024;
+ nbs = *bls + 1024;
+ if ((blt = resalloc(bl, nbs, sizeof(struct bl))) == NULL) {
  serrno = errno;
  goto bldone;
  }
+ *bls = nbs;
  bl = blt;
  }
  if (i == bu || buf[i] == '\n') {
@@ -506,8 +537,7 @@
  if (bu == 0)
  errno = EIO;
  bldone:
- if (buf)
- free(buf);
+ free(buf);
  if (serrno)
  errno = serrno;
  return (bl);
@@ -530,7 +560,7 @@
  * printable form to pfctl or spamd.
  */
 struct cidr **
-collapse_blacklist(struct bl *bl, int blc)
+collapse_blacklist(struct bl *bl, size_t blc)
 {
  int bs = 0, ws = 0, state=0, cli, i;
  u_int32_t bstart = 0;
@@ -538,12 +568,10 @@
  int laststate;
  u_int32_t addr;
 
- if (blc == 0)
- return (NULL);
- cl = malloc((blc / 2) * sizeof(struct cidr));
- if (cl == NULL) {
- return (NULL);
- }
+ if (blc < 2)
+ errx(1, "blacklist too small");
+ if ((cl = calloc(blc / 2, sizeof(struct cidr))) == NULL)
+ err(1, "calloc");
  qsort(bl, blc, sizeof(struct bl), cmpbl);
  cli = 0;
  cl[cli] = NULL;
@@ -664,11 +692,12 @@
 }
 
 int
-getlist(char ** db_array, char *name, struct blacklist *blist,
+getlist(char **db_array, char *name, struct blacklist *blist,
     struct blacklist *blistnew)
 {
  char *buf, *method, *file, *message;
- int blc, bls, fd, black = 0;
+ int fd, black = 0;
+ size_t blc, bls;
  struct bl *bl = NULL;
  gzFile gzf;
 
@@ -745,7 +774,7 @@
  blist->bls = bls;
  }
  if (debug)
- fprintf(stderr, "%slist %s %d entries\n",
+ fprintf(stderr, "%slist %s %lu entries\n",
     black ? "black" : "white", name, blc / 2);
  return (black);
 }
@@ -753,11 +782,12 @@
 int
 main(int argc, char *argv[])
 {
- size_t dbs, dbc, blc, bls, black, white;
+ size_t dbs, dbc, blc, bls, black, white, nbls;
  char **db_array, *buf, *name;
- struct blacklist *blists;
+ struct blacklist *blists, *blt;
  struct servent *ent;
  int i, ch;
+ struct cidr **cidrs, **tmp;
 
  while ((ch = getopt(argc, argv, "nd")) != -1) {
  switch (ch) {
@@ -778,9 +808,8 @@
 
  dbs = argc + 2;
  dbc = 0;
- db_array = calloc(dbs, sizeof(char *));
- if (db_array == NULL)
- errx(1, "malloc failed");
+ if ((db_array = calloc(dbs, sizeof(char *))) == NULL)
+ err(1, "calloc");
 
  db_array[dbc]= PATH_SPAMD_CONF;
  dbc++;
@@ -797,14 +826,12 @@
  if (*name) {
  /* extract config in order specified in "all" tag */
  if (blc == bls) {
- struct blacklist *tmp;
-
- bls += 1024;
- tmp = realloc(blists,
-    bls * sizeof(struct blacklist));
- if (tmp == NULL)
- errx(1, "malloc failed");
- blists = tmp;
+ nbls = bls + 1024;
+ if ((blt = resalloc(blists, nbls,
+    sizeof(struct blacklist))) == NULL)
+ err(1, "realloc");
+ blists = blt;
+ bls = nbls;
  }
  if (blc == 0)
  black = white = 0;
@@ -818,13 +845,9 @@
  }
  }
  for (i = 0; i < blc; i++) {
- struct cidr **cidrs, **tmp;
-
  if (blists[i].blc > 0) {
  cidrs = collapse_blacklist(blists[i].bl,
    blists[i].blc);
- if (cidrs == NULL)
- errx(1, "malloc failed");
  if (dryrun)
  continue;