From: Omar Polo Subject: Re: smtpd: unify the table(5) parser with makemap To: "Todd C. Miller" Cc: tech@openbsd.org Date: Thu, 08 Feb 2024 10:11:25 +0100 On 2024/02/06 11:05:30 -0700, Todd C. Miller wrote: > Looks good to me, two comments inline. Thanks for reviewing > > --- makemap.c > > +++ makemap.c > > [...] > > @@ -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(). Oops, yes, fixed, thanks. > > size_t lineno = 0; > > + int malformed, table_type, r; > > > > if (strcmp(filename, "-") == 0) > > fp = fdopen(0, "r"); > > [...] > > --- util.c > > +++ util.c > > [...] > > + 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. Completely agree. I was tempted to change it, then haven't to avoid introducing differences while moving code, but I prefer having an extra bounds check here too. updated diff 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 = 0; 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 (linelen > 0 && isspace((unsigned char)keyp[linelen - 1])) + keyp[--linelen] = '\0'; + 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); +}