From: schwarze Date: Wed, 23 Apr 2014 16:07:06 +0000 (+0000) Subject: Audit strlcpy(3)/strlcat(3) usage. X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=0b2f1307db21250b7099790c5dac4bfa2b206352;p=openbsd Audit strlcpy(3)/strlcat(3) usage. * Repair three instances of silent truncation, use asprintf(3). * Change two instances of strlen(3)+malloc(3)+strlcpy(3)+strlcat(3)+... to use asprintf(3) instead to make them less error prone. * Cast the return value of four instances where the destination buffer is known to be large enough to (void). * Completely remove three useless instances of strlcpy(3)/strlcat(3). * Mark two places in -Thtml with XXX that can cause information loss and crashes but are not easy to fix, requiring design changes of some internal interfaces. * The file mandocdb.c remains to be audited. --- diff --git a/usr.bin/mandoc/html.c b/usr.bin/mandoc/html.c index c304b80b091..50a1f7ac1f5 100644 --- a/usr.bin/mandoc/html.c +++ b/usr.bin/mandoc/html.c @@ -1,4 +1,4 @@ -/* $Id: html.c,v 1.35 2014/04/20 16:44:44 schwarze Exp $ */ +/* $Id: html.c,v 1.36 2014/04/23 16:07:06 schwarze Exp $ */ /* * Copyright (c) 2008, 2009, 2010, 2011 Kristaps Dzonsons * Copyright (c) 2011, 2012, 2013, 2014 Ingo Schwarze @@ -653,6 +653,12 @@ void bufcat(struct html *h, const char *p) { + /* + * XXX This is broken and not easy to fix. + * When using the -Oincludes option, buffmt_includes() + * may pass in strings overrunning BUFSIZ, causing a crash. + */ + h->buflen = strlcat(h->buf, p, BUFSIZ); assert(h->buflen < BUFSIZ); } diff --git a/usr.bin/mandoc/man_html.c b/usr.bin/mandoc/man_html.c index 8df9f91e7b4..e14daa503a6 100644 --- a/usr.bin/mandoc/man_html.c +++ b/usr.bin/mandoc/man_html.c @@ -1,4 +1,4 @@ -/* $Id: man_html.c,v 1.53 2014/04/20 20:17:36 schwarze Exp $ */ +/* $Id: man_html.c,v 1.54 2014/04/23 16:07:06 schwarze Exp $ */ /* * Copyright (c) 2008-2012 Kristaps Dzonsons * Copyright (c) 2013, 2014 Ingo Schwarze @@ -297,15 +297,10 @@ a2width(const struct man_node *n, struct roffsu *su) static void man_root_pre(MAN_ARGS) { - char b[BUFSIZ]; struct htmlpair tag[3]; struct tag *t, *tt; char *title; - b[0] = 0; - if (man->vol) - (void)strlcat(b, man->vol, BUFSIZ); - assert(man->title); assert(man->msec); mandoc_asprintf(&title, "%s(%s)", man->title, man->msec); @@ -331,7 +326,8 @@ man_root_pre(MAN_ARGS) PAIR_CLASS_INIT(&tag[0], "head-vol"); PAIR_INIT(&tag[1], ATTR_ALIGN, "center"); print_otag(h, TAG_TD, 2, tag); - print_text(h, b); + if (NULL != man->vol) + print_text(h, man->vol); print_stagq(h, tt); PAIR_CLASS_INIT(&tag[0], "head-rtitle"); diff --git a/usr.bin/mandoc/man_term.c b/usr.bin/mandoc/man_term.c index 92a2b76f473..667240679c8 100644 --- a/usr.bin/mandoc/man_term.c +++ b/usr.bin/mandoc/man_term.c @@ -1,4 +1,4 @@ -/* $Id: man_term.c,v 1.101 2014/04/20 20:17:36 schwarze Exp $ */ +/* $Id: man_term.c,v 1.102 2014/04/23 16:07:06 schwarze Exp $ */ /* * Copyright (c) 2008-2012 Kristaps Dzonsons * Copyright (c) 2010-2014 Ingo Schwarze @@ -1115,20 +1115,17 @@ print_man_foot(struct termp *p, const void *arg) static void print_man_head(struct termp *p, const void *arg) { - char buf[BUFSIZ]; const struct man_meta *meta; + const char *volume; char *title; - size_t buflen, titlen; + size_t vollen, titlen; meta = (const struct man_meta *)arg; assert(meta->title); assert(meta->msec); - if (meta->vol) - strlcpy(buf, meta->vol, BUFSIZ); - else - buf[0] = '\0'; - buflen = term_strlen(p, buf); + volume = NULL == meta->vol ? "" : meta->vol; + vollen = term_strlen(p, volume); /* Top left corner: manual title and section. */ @@ -1138,10 +1135,9 @@ print_man_head(struct termp *p, const void *arg) p->flags |= TERMP_NOBREAK | TERMP_NOSPACE; p->trailspace = 1; p->offset = 0; - p->rmargin = 2 * (titlen+1) + buflen < p->maxrmargin ? - (p->maxrmargin - - term_strlen(p, buf) + term_len(p, 1)) / 2 : - p->maxrmargin - buflen; + p->rmargin = 2 * (titlen+1) + vollen < p->maxrmargin ? + (p->maxrmargin - vollen + term_len(p, 1)) / 2 : + p->maxrmargin - vollen; term_word(p, title); term_flushln(p); @@ -1150,10 +1146,10 @@ print_man_head(struct termp *p, const void *arg) p->flags |= TERMP_NOSPACE; p->offset = p->rmargin; - p->rmargin = p->offset + buflen + titlen < p->maxrmargin ? + p->rmargin = p->offset + vollen + titlen < p->maxrmargin ? p->maxrmargin - titlen : p->maxrmargin; - term_word(p, buf); + term_word(p, volume); term_flushln(p); /* Top right corner: title and section, again. */ diff --git a/usr.bin/mandoc/mdoc_html.c b/usr.bin/mandoc/mdoc_html.c index 5c44f7f650f..7ceb9e2396b 100644 --- a/usr.bin/mandoc/mdoc_html.c +++ b/usr.bin/mandoc/mdoc_html.c @@ -1,4 +1,4 @@ -/* $Id: mdoc_html.c,v 1.72 2014/04/20 20:17:36 schwarze Exp $ */ +/* $Id: mdoc_html.c,v 1.73 2014/04/23 16:07:06 schwarze Exp $ */ /* * Copyright (c) 2008, 2009, 2010, 2011 Kristaps Dzonsons * Copyright (c) 2014 Ingo Schwarze @@ -511,18 +511,15 @@ mdoc_root_post(MDOC_ARGS) static int mdoc_root_pre(MDOC_ARGS) { - char b[BUFSIZ]; struct htmlpair tag[3]; struct tag *t, *tt; - char *title; + char *volume, *title; - strlcpy(b, meta->vol, BUFSIZ); - - if (meta->arch) { - strlcat(b, " (", BUFSIZ); - strlcat(b, meta->arch, BUFSIZ); - strlcat(b, ")", BUFSIZ); - } + if (NULL == meta->arch) + volume = mandoc_strdup(meta->vol); + else + mandoc_asprintf(&volume, "%s (%s)", + meta->vol, meta->arch); mandoc_asprintf(&title, "%s(%s)", meta->title, meta->msec); @@ -547,7 +544,7 @@ mdoc_root_pre(MDOC_ARGS) PAIR_CLASS_INIT(&tag[0], "head-vol"); PAIR_INIT(&tag[1], ATTR_ALIGN, "center"); print_otag(h, TAG_TD, 2, tag); - print_text(h, b); + print_text(h, volume); print_stagq(h, tt); PAIR_CLASS_INIT(&tag[0], "head-rtitle"); @@ -557,6 +554,7 @@ mdoc_root_pre(MDOC_ARGS) print_tagq(h, t); free(title); + free(volume); return(1); } @@ -989,8 +987,8 @@ mdoc_bl_pre(MDOC_ARGS) PAIR_STYLE_INIT(&tag[0], h); assert(lists[n->norm->Bl.type]); - strlcpy(buf, "list ", BUFSIZ); - strlcat(buf, lists[n->norm->Bl.type], BUFSIZ); + (void)strlcpy(buf, "list ", BUFSIZ); + (void)strlcat(buf, lists[n->norm->Bl.type], BUFSIZ); PAIR_INIT(&tag[1], ATTR_CLASS, buf); /* Set the block's left-hand margin. */ @@ -1359,6 +1357,15 @@ mdoc_fd_pre(MDOC_ARGS) if (NULL != (n = n->next)) { assert(MDOC_TEXT == n->type); + + /* + * XXX This is broken and not easy to fix. + * When using -Oincludes, truncation may occur. + * Dynamic allocation wouldn't help because + * passing long strings to buffmt_includes() + * does not work either. + */ + strlcpy(buf, '<' == *n->string || '"' == *n->string ? n->string + 1 : n->string, BUFSIZ); @@ -1471,10 +1478,8 @@ mdoc_fn_pre(MDOC_ARGS) t = print_otag(h, TAG_B, 1, tag); - if (sp) { - strlcpy(nbuf, sp, BUFSIZ); - print_text(h, nbuf); - } + if (sp) + print_text(h, sp); print_tagq(h, t); diff --git a/usr.bin/mandoc/mdoc_term.c b/usr.bin/mandoc/mdoc_term.c index 4f7d9d0effe..8dde546e930 100644 --- a/usr.bin/mandoc/mdoc_term.c +++ b/usr.bin/mandoc/mdoc_term.c @@ -1,4 +1,4 @@ -/* $Id: mdoc_term.c,v 1.168 2014/04/20 20:17:36 schwarze Exp $ */ +/* $Id: mdoc_term.c,v 1.169 2014/04/23 16:07:06 schwarze Exp $ */ /* * Copyright (c) 2008, 2009, 2010, 2011 Kristaps Dzonsons * Copyright (c) 2010, 2012, 2013, 2014 Ingo Schwarze @@ -438,10 +438,9 @@ print_mdoc_foot(struct termp *p, const void *arg) static void print_mdoc_head(struct termp *p, const void *arg) { - char buf[BUFSIZ]; const struct mdoc_meta *meta; - char *title; - size_t buflen, titlen; + char *volume, *title; + size_t vollen, titlen; meta = (const struct mdoc_meta *)arg; @@ -462,14 +461,12 @@ print_mdoc_head(struct termp *p, const void *arg) p->rmargin = p->maxrmargin; assert(meta->vol); - strlcpy(buf, meta->vol, BUFSIZ); - buflen = term_strlen(p, buf); - - if (meta->arch) { - strlcat(buf, " (", BUFSIZ); - strlcat(buf, meta->arch, BUFSIZ); - strlcat(buf, ")", BUFSIZ); - } + if (NULL == meta->arch) + volume = mandoc_strdup(meta->vol); + else + mandoc_asprintf(&volume, "%s (%s)", + meta->vol, meta->arch); + vollen = term_strlen(p, volume); mandoc_asprintf(&title, "%s(%s)", meta->title, meta->msec); titlen = term_strlen(p, title); @@ -477,20 +474,19 @@ print_mdoc_head(struct termp *p, const void *arg) p->flags |= TERMP_NOBREAK | TERMP_NOSPACE; p->trailspace = 1; p->offset = 0; - p->rmargin = 2 * (titlen+1) + buflen < p->maxrmargin ? - (p->maxrmargin - - term_strlen(p, buf) + term_len(p, 1)) / 2 : - p->maxrmargin - buflen; + p->rmargin = 2 * (titlen+1) + vollen < p->maxrmargin ? + (p->maxrmargin - vollen + term_len(p, 1)) / 2 : + p->maxrmargin - vollen; term_word(p, title); term_flushln(p); p->flags |= TERMP_NOSPACE; p->offset = p->rmargin; - p->rmargin = p->offset + buflen + titlen < p->maxrmargin ? + p->rmargin = p->offset + vollen + titlen < p->maxrmargin ? p->maxrmargin - titlen : p->maxrmargin; - term_word(p, buf); + term_word(p, volume); term_flushln(p); p->flags &= ~TERMP_NOBREAK; @@ -507,6 +503,7 @@ print_mdoc_head(struct termp *p, const void *arg) p->offset = 0; p->rmargin = p->maxrmargin; free(title); + free(volume); } static size_t diff --git a/usr.bin/mandoc/mdoc_validate.c b/usr.bin/mandoc/mdoc_validate.c index 297769d0a53..50aa7bce14d 100644 --- a/usr.bin/mandoc/mdoc_validate.c +++ b/usr.bin/mandoc/mdoc_validate.c @@ -1,4 +1,4 @@ -/* $Id: mdoc_validate.c,v 1.130 2014/04/20 20:48:34 schwarze Exp $ */ +/* $Id: mdoc_validate.c,v 1.131 2014/04/23 16:07:06 schwarze Exp $ */ /* * Copyright (c) 2008-2012 Kristaps Dzonsons * Copyright (c) 2010-2014 Ingo Schwarze @@ -1179,9 +1179,9 @@ post_defaults(POST_ARGS) static int post_at(POST_ARGS) { - const char *p, *q; - char *buf; - size_t sz; + struct mdoc_node *n; + const char *std_att; + char *att; /* * If we have a child, look it up in the standard keys. If a @@ -1189,27 +1189,18 @@ post_at(POST_ARGS) * prefix "AT&T UNIX " to the existing data. */ - if (NULL == mdoc->last->child) + if (NULL == (n = mdoc->last->child)) return(1); - assert(MDOC_TEXT == mdoc->last->child->type); - p = mdoc_a2att(mdoc->last->child->string); - - if (p) { - free(mdoc->last->child->string); - mdoc->last->child->string = mandoc_strdup(p); - } else { + assert(MDOC_TEXT == n->type); + if (NULL == (std_att = mdoc_a2att(n->string))) { mdoc_nmsg(mdoc, mdoc->last, MANDOCERR_BADATT); - p = "AT&T UNIX "; - q = mdoc->last->child->string; - sz = strlen(p) + strlen(q) + 1; - buf = mandoc_malloc(sz); - strlcpy(buf, p, sz); - strlcat(buf, q, sz); - free(mdoc->last->child->string); - mdoc->last->child->string = buf; - } + mandoc_asprintf(&att, "AT&T UNIX %s", n->string); + } else + att = mandoc_strdup(std_att); + free(n->string); + n->string = att; return(1); } diff --git a/usr.bin/mandoc/roff.c b/usr.bin/mandoc/roff.c index 0b05d955401..41859f35644 100644 --- a/usr.bin/mandoc/roff.c +++ b/usr.bin/mandoc/roff.c @@ -1,4 +1,4 @@ -/* $Id: roff.c,v 1.81 2014/04/20 19:39:35 schwarze Exp $ */ +/* $Id: roff.c,v 1.82 2014/04/23 16:07:06 schwarze Exp $ */ /* * Copyright (c) 2010, 2011, 2012 Kristaps Dzonsons * Copyright (c) 2010-2014 Ingo Schwarze @@ -486,14 +486,13 @@ roff_res(struct roff *r, char **bufp, size_t *szp, int ln, int pos) { char ubuf[24]; /* buffer to print the number */ const char *start; /* start of the string to process */ - const char *stesc; /* start of an escape sequence ('\\') */ + char *stesc; /* start of an escape sequence ('\\') */ const char *stnam; /* start of the name, after "[(*" */ const char *cp; /* end of the name, e.g. before ']' */ const char *res; /* the string to be substituted */ char *nbuf; /* new buffer to copy bufp to */ size_t maxl; /* expected length of the escape name */ size_t naml; /* actual length of the escape name */ - size_t ressz; /* size of the replacement string */ int expand_count; /* to avoid infinite loops */ int npos; /* position in numeric expression */ int irc; /* return code from roff_evalnum() */ @@ -516,7 +515,7 @@ roff_res(struct roff *r, char **bufp, size_t *szp, int ln, int pos) break; if (0 == (stesc - cp) % 2) { - stesc = cp; + stesc = (char *)cp; continue; } @@ -624,21 +623,17 @@ roff_res(struct roff *r, char **bufp, size_t *szp, int ln, int pos) ln, (int)(stesc - *bufp), NULL); res = ""; } - ressz = strlen(res); /* Replace the escape sequence by the string. */ - *szp += ressz + 1; - nbuf = mandoc_malloc(*szp); - - strlcpy(nbuf, *bufp, (size_t)(stesc - *bufp + 1)); - strlcat(nbuf, res, *szp); - strlcat(nbuf, cp, *szp); + *stesc = '\0'; + *szp = mandoc_asprintf(&nbuf, "%s%s%s", + *bufp, res, cp) + 1; /* Prepare for the next replacement. */ start = nbuf + pos; - stesc = nbuf + (stesc - *bufp) + ressz; + stesc = nbuf + (stesc - *bufp) + strlen(res); free(*bufp); *bufp = nbuf; } @@ -1986,14 +1981,9 @@ roff_userdef(ROFF_ARGS) cp += 2; continue; } - - *szp = strlen(n1) - 3 + strlen(arg[i]) + 1; - n2 = mandoc_malloc(*szp); - - strlcpy(n2, n1, (size_t)(cp - n1 + 1)); - strlcat(n2, arg[i], *szp); - strlcat(n2, cp + 3, *szp); - + *cp = '\0'; + *szp = mandoc_asprintf(&n2, "%s%s%s", + n1, arg[i], cp + 3) + 1; cp = n2 + (cp - n1); free(n1); n1 = n2; diff --git a/usr.bin/mandoc/tbl_data.c b/usr.bin/mandoc/tbl_data.c index 4031cd84131..d41c7642e78 100644 --- a/usr.bin/mandoc/tbl_data.c +++ b/usr.bin/mandoc/tbl_data.c @@ -1,4 +1,4 @@ -/* $Id: tbl_data.c,v 1.17 2014/04/20 16:44:44 schwarze Exp $ */ +/* $Id: tbl_data.c,v 1.18 2014/04/23 16:07:06 schwarze Exp $ */ /* * Copyright (c) 2009, 2010, 2011 Kristaps Dzonsons * Copyright (c) 2011 Ingo Schwarze @@ -163,8 +163,8 @@ tbl_cdata(struct tbl_node *tbl, int ln, const char *p) if (dat->string) { sz = strlen(p) + strlen(dat->string) + 2; dat->string = mandoc_realloc(dat->string, sz); - strlcat(dat->string, " ", sz); - strlcat(dat->string, p, sz); + (void)strlcat(dat->string, " ", sz); + (void)strlcat(dat->string, p, sz); } else dat->string = mandoc_strdup(p);