From: Todd C. Miller Subject: Re: smtpd: unify the table(5) parser with makemap To: Omar Polo Cc: tech@openbsd.org Date: Tue, 06 Feb 2024 11:05:30 -0700 Looks good to me, two comments inline. - todd > diff /home/op/tmp/smtpd > commit - 1f95d9310cff81a3ddd7b95069ea5dea8d1b03de > path + /home/op/tmp/smtpd > blob - 2144e83959e9c25b24fc0913b0907a5af86a3804 > file + makemap.c > --- makemap.c > +++ makemap.c > @@ -37,9 +37,8 @@ > > static void usage(void); > static int parse_map(DB *, int *, char *); > -static int parse_entry(DB *, int *, char *, size_t, size_t); > -static int parse_mapentry(DB *, int *, char *, size_t, size_t); > -static int parse_setentry(DB *, int *, char *, size_t, size_t); > +static int add_mapentry(DB *, int *, char *, char *, size_t); > +static int add_setentry(DB *, int *, char *, size_t); > static int make_plain(DBT *, char *); > static int make_aliases(DBT *, char *); > static char *conf_aliases(char *); > @@ -247,9 +246,10 @@ static int > parse_map(DB *db, int *dbputs, char *filename) > { > FILE *fp; > - char *line; > - size_t len; > + char *key, *val, *line = NULL; > + size_t linesize; You may want to set linesize=0 here for consistency. It is not strictly required since line is NULL, but you do zero it out in the other consumer of parse_table_line(). > size_t lineno = 0; > + int malformed, table_type, r; > > if (strcmp(filename, "-") == 0) > fp = fdopen(0, "r"); > @@ -269,56 +269,47 @@ parse_map(DB *db, int *dbputs, char *filename) > return 0; > } > > - while ((line = fparseln(fp, &len, &lineno, > - NULL, FPARSELN_UNESCCOMM)) != NULL) { > - if (!parse_entry(db, dbputs, line, len, lineno)) { > + table_type = (type == T_SET) ? T_LIST : T_HASH; > + while (parse_table_line(fp, &line, &linesize, &table_type, > + &key, &val, &malformed) != -1) { > + lineno++; > + if (malformed) { > + warnx("%s:%zd: invalid entry", source, lineno); > free(line); > fclose(fp); > return 0; > } > - free(line); > + if (key == NULL) > + continue; > + > + switch (type) { > + case T_PLAIN: > + case T_ALIASES: > + r = add_mapentry(db, dbputs, key, val, lineno); > + break; > + case T_SET: > + r = add_setentry(db, dbputs, key, lineno); > + break; > + } > + > + if (!r) { > + free(line); > + fclose(fp); > + return 0; > + } > } > > + free(line); > fclose(fp); > return 1; > } > > static int > -parse_entry(DB *db, int *dbputs, char *line, size_t len, size_t lineno) > +add_mapentry(DB *db, int *dbputs, char *keyp, char *valp, size_t lineno) > { > - switch (type) { > - case T_PLAIN: > - case T_ALIASES: > - return parse_mapentry(db, dbputs, line, len, lineno); > - case T_SET: > - return parse_setentry(db, dbputs, line, len, lineno); > - } > - return 0; > -} > - > -static int > -parse_mapentry(DB *db, int *dbputs, char *line, size_t len, size_t lineno) > -{ > DBT key; > DBT val; > - char *keyp; > - char *valp; > > - keyp = line; > - while (isspace((unsigned char)*keyp)) > - keyp++; > - if (*keyp == '\0') > - return 1; > - > - valp = keyp; > - strsep(&valp, " \t:"); > - if (valp == NULL || valp == keyp) > - goto bad; > - while (*valp == ':' || isspace((unsigned char)*valp)) > - valp++; > - if (*valp == '\0') > - goto bad; > - > /* Check for dups. */ > key.data = keyp; > key.size = strlen(keyp) + 1; > @@ -355,18 +346,11 @@ bad: > } > > static int > -parse_setentry(DB *db, int *dbputs, char *line, size_t len, size_t lineno) > +add_setentry(DB *db, int *dbputs, char *keyp, size_t lineno) > { > DBT key; > DBT val; > - char *keyp; > > - keyp = line; > - while (isspace((unsigned char)*keyp)) > - keyp++; > - if (*keyp == '\0') > - return 1; > - > val.data = ""; > val.size = strlen(val.data) + 1; > > blob - 25090c3acf9980c0fbe22784a5242dae64c8ec53 > file + smtpd.h > --- smtpd.h > +++ smtpd.h > @@ -1738,6 +1738,9 @@ void log_trace0(const char *, ...) > __attribute__((format (printf, 1, 2))); > #define log_trace(m, ...) do { if (tracing & (m)) log_trace0(__VA_ARGS__); > } while (0) > > +int parse_table_line(FILE *, char **, size_t *, int *, > + char **, char **, int *); > + > /* waitq.c */ > int waitq_wait(void *, void (*)(void *, void *, void *), void *); > void waitq_run(void *, void *); > blob - af57038f2313f787ad14ab9156a97a2487df87b4 > file + table_static.c > --- table_static.c > +++ table_static.c > @@ -111,80 +111,28 @@ static int > table_static_priv_load(struct table_static_priv *priv, const char *path) > { > FILE *fp; > - char *buf = NULL, *p; > + char *line = NULL; > int lineno = 0; > - size_t sz = 0; > - ssize_t flen; > + size_t linesize = 0; > char *keyp; > char *valp; > - int ret = 0; > + int malformed, ret = 0; > > if ((fp = fopen(path, "r")) == NULL) { > log_warn("%s: fopen", path); > return 0; > } > > - while ((flen = getline(&buf, &sz, fp)) != -1) { > + while (parse_table_line(fp, &line, &linesize, &priv->type, > + &keyp, &valp, &malformed) != -1) { > lineno++; > - if (buf[flen - 1] == '\n') > - buf[--flen] = '\0'; > - > - keyp = buf; > - while (isspace((unsigned char)*keyp)) { > - ++keyp; > - --flen; > - } > - if (*keyp == '\0') > - continue; > - while (isspace((unsigned char)keyp[flen - 1])) > - keyp[--flen] = '\0'; > - if (*keyp == '#') { > - if (priv->type == T_NONE) { > - keyp++; > - while (isspace((unsigned char)*keyp)) > - ++keyp; > - if (!strcmp(keyp, "@list")) > - priv->type = T_LIST; > - } > - continue; > - } > - > - if (priv->type == T_NONE) { > - for (p = keyp; *p; p++) { > - if (*p == ' ' || *p == '\t' || *p == ':') { > - priv->type = T_HASH; > - break; > - } > - } > - if (priv->type == T_NONE) > - priv->type = T_LIST; > - } > - > - if (priv->type == T_LIST) { > - table_static_priv_add(priv, keyp, NULL); > - continue; > - } > - > - /* T_HASH */ > - valp = keyp; > - strsep(&valp, " \t:"); > - if (valp) { > - while (*valp) { > - if (!isspace((unsigned char)*valp) && > - !(*valp == ':' && > - isspace((unsigned char)*(valp + 1)))) > - break; > - ++valp; > - } > - if (*valp == '\0') > - valp = NULL; > - } > - if (valp == NULL) { > - log_warnx("%s: invalid map entry line %d", > + if (malformed) { > + log_warnx("%s:%d invalid map entry", > path, lineno); > goto end; > } > - > + if (keyp == NULL) > + continue; > table_static_priv_add(priv, keyp, valp); > } > > @@ -199,7 +147,7 @@ table_static_priv_load(struct table_static_priv *priv, > > ret = 1; > end: > - free(buf); > + free(line); > fclose(fp); > return ret; > } > blob - feb663cc61ec2572b57741fd694926f0c26967c7 > file + util.c > --- util.c > +++ util.c > @@ -826,3 +826,75 @@ log_trace_verbose(int v) > /* Set debug logging in log.c */ > log_setverbose(v & TRACE_DEBUG ? 2 : foreground_log); > } > + > +int > +parse_table_line(FILE *fp, char **line, size_t *linesize, > + int *type, char **key, char **val, int *malformed) > +{ > + char *keyp, *valp, *p; > + ssize_t linelen; > + > + *key = NULL; > + *val = NULL; > + *malformed = 0; > + > + if ((linelen = getline(line, linesize, fp)) == -1) > + return (-1); > + > + keyp = *line; > + while (isspace((unsigned char)*keyp)) { > + ++keyp; > + --linelen; > + } > + if (*keyp == '\0') > + return 0; > + while (isspace((unsigned char)keyp[linelen - 1])) > + keyp[--linelen] = '\0'; I would feel better if this was: while (linelen > 0 && isspace((unsigned char)keyp[linelen - 1])) keyp[--linelen] = '\0'; I realize it should not be possible for linelen to go negative here because you've already trimmed leading space, but it doesn't hurt to be extra careful. It will probably also save you from a static analyzer complaint. > + if (*keyp == '#') { > + if (*type == T_NONE) { > + keyp++; > + while (isspace((unsigned char)*keyp)) > + ++keyp; > + if (!strcmp(keyp, "@list")) > + *type = T_LIST; > + } > + return 0; > + } > + > + if (*type == T_NONE) { > + for (p = keyp; *p; p++) { > + if (*p == ' ' || *p == '\t' || *p == ':') { > + *type = T_HASH; > + break; > + } > + } > + if (*type == T_NONE) > + *type = T_LIST; > + } > + > + if (*type == T_LIST) { > + *key = keyp; > + return (0); > + } > + > + /* T_HASH */ > + valp = keyp; > + strsep(&valp, " \t:"); > + if (valp) { > + while (*valp) { > + if (!isspace((unsigned char)*valp) && > + !(*valp == ':' && > + isspace((unsigned char)*(valp + 1)))) > + break; > + ++valp; > + } > + if (*valp == '\0') > + valp = NULL; > + } > + if (valp == NULL) > + *malformed = 1; > + > + *key = keyp; > + *val = valp; > + return (0); > +} >