]> git.cameronkatri.com Git - mandoc.git/commitdiff
Audit strlcpy(3)/strlcat(3) usage.
authorIngo Schwarze <schwarze@openbsd.org>
Wed, 23 Apr 2014 16:08:33 +0000 (16:08 +0000)
committerIngo Schwarze <schwarze@openbsd.org>
Wed, 23 Apr 2014 16:08:33 +0000 (16:08 +0000)
* 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.

TODO
html.c
man_html.c
man_term.c
mdoc_html.c
mdoc_term.c
mdoc_validate.c
roff.c
tbl_data.c

diff --git a/TODO b/TODO
index e619f2729b04f15d711704d0b9c2944a5c2861a0..2d4f2622663e82fd1b1f38dfada36ed88f0c876f 100644 (file)
--- a/TODO
+++ b/TODO
@@ -1,13 +1,15 @@
 ************************************************************************
 * Official mandoc TODO.
-* $Id: TODO,v 1.168 2014/03/30 19:47:48 schwarze Exp $
+* $Id: TODO,v 1.169 2014/04/23 16:08:33 schwarze Exp $
 ************************************************************************
 
 ************************************************************************
 * crashes
 ************************************************************************
 
-None known.
+- The abort() in bufcat(), html.c, can be triggered via buffmt_includes()
+  by running -Thtml -Oincludes on a file containing a long .In argument.
+  Fixing this will probably require reworking the whole bufcat() concept.
 
 ************************************************************************
 * missing features
diff --git a/html.c b/html.c
index 7319de66790264f0774ce3072b67843393e57498..b8a4c44512232511cecd99e3a6c262f9d2881c8e 100644 (file)
--- a/html.c
+++ b/html.c
@@ -1,4 +1,4 @@
-/*     $Id: html.c,v 1.156 2014/04/20 16:46:04 schwarze Exp $ */
+/*     $Id: html.c,v 1.157 2014/04/23 16:08:33 schwarze Exp $ */
 /*
  * Copyright (c) 2008, 2009, 2010, 2011 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2011, 2012, 2013, 2014 Ingo Schwarze <schwarze@openbsd.org>
@@ -657,6 +657,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);
 }
index 092c4c21104c05adc1b425ddbc7998cfbe56a49e..5df9e4697d80ea78a54cca69bc31720413637ae8 100644 (file)
@@ -1,4 +1,4 @@
-/*     $Id: man_html.c,v 1.94 2014/04/20 20:18:12 schwarze Exp $ */
+/*     $Id: man_html.c,v 1.95 2014/04/23 16:08:33 schwarze Exp $ */
 /*
  * Copyright (c) 2008-2012 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2013, 2014 Ingo Schwarze <schwarze@openbsd.org>
@@ -301,15 +301,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);
@@ -335,7 +330,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");
index f3fc411483a308f9733d5af66dbf84d3607a4c3d..e308f6a381ae3f52f2cee8e95f215e024df42e4f 100644 (file)
@@ -1,4 +1,4 @@
-/*     $Id: man_term.c,v 1.147 2014/04/20 20:18:12 schwarze Exp $ */
+/*     $Id: man_term.c,v 1.148 2014/04/23 16:08:33 schwarze Exp $ */
 /*
  * Copyright (c) 2008-2012 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2010-2014 Ingo Schwarze <schwarze@openbsd.org>
@@ -1119,20 +1119,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. */
 
@@ -1142,10 +1139,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);
@@ -1154,10 +1150,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. */
index ac1e43b44a469f60cf699e53e13fdcb552975882..2dc15c53c04dc6c258eb64a473458ca5a7d92c94 100644 (file)
@@ -1,4 +1,4 @@
-/*     $Id: mdoc_html.c,v 1.189 2014/04/20 20:18:12 schwarze Exp $ */
+/*     $Id: mdoc_html.c,v 1.190 2014/04/23 16:08:33 schwarze Exp $ */
 /*
  * Copyright (c) 2008, 2009, 2010, 2011 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2014 Ingo Schwarze <schwarze@openbsd.org>
@@ -515,18 +515,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);
 
@@ -551,7 +548,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");
@@ -561,6 +558,7 @@ mdoc_root_pre(MDOC_ARGS)
        print_tagq(h, t);
 
        free(title);
+       free(volume);
        return(1);
 }
 
@@ -993,8 +991,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. */
@@ -1363,6 +1361,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);
 
@@ -1475,10 +1482,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);
 
index fac902bcd93d60e060c28a77a78b9c20eefdbd7a..93bb05eaa383fce86e96f80ce069bd6e96bc74c7 100644 (file)
@@ -1,4 +1,4 @@
-/*     $Id: mdoc_term.c,v 1.266 2014/04/20 20:18:12 schwarze Exp $ */
+/*     $Id: mdoc_term.c,v 1.267 2014/04/23 16:08:33 schwarze Exp $ */
 /*
  * Copyright (c) 2008, 2009, 2010, 2011 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2010, 2012, 2013, 2014 Ingo Schwarze <schwarze@openbsd.org>
@@ -442,10 +442,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;
 
@@ -466,14 +465,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);
@@ -481,20 +478,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;
@@ -511,6 +507,7 @@ print_mdoc_head(struct termp *p, const void *arg)
        p->offset = 0;
        p->rmargin = p->maxrmargin;
        free(title);
+       free(volume);
 }
 
 static size_t
index 55573fa8c9f9a1384f9796cded1f671b17a1df49..8c394328ab3048b13ea1c134778e3ceed620ca81 100644 (file)
@@ -1,4 +1,4 @@
-/*     $Id: mdoc_validate.c,v 1.212 2014/04/20 20:48:53 schwarze Exp $ */
+/*     $Id: mdoc_validate.c,v 1.213 2014/04/23 16:08:33 schwarze Exp $ */
 /*
  * Copyright (c) 2008-2012 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2010-2014 Ingo Schwarze <schwarze@openbsd.org>
@@ -1183,9 +1183,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
@@ -1193,27 +1193,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/roff.c b/roff.c
index fee1a8c1ec80b21409d82e36d699abb5cca8e432..07480a12c44bc5cafc5f56b828553b35f72928e9 100644 (file)
--- a/roff.c
+++ b/roff.c
@@ -1,4 +1,4 @@
-/*     $Id: roff.c,v 1.208 2014/04/20 19:40:13 schwarze Exp $ */
+/*     $Id: roff.c,v 1.209 2014/04/23 16:08:33 schwarze Exp $ */
 /*
  * Copyright (c) 2010, 2011, 2012 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2010-2014 Ingo Schwarze <schwarze@openbsd.org>
@@ -490,14 +490,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() */
@@ -520,7 +519,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;
                }
 
@@ -628,21 +627,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;
        }
@@ -1990,14 +1985,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;
index 2a362e5c5d4bb3e1e2ff376267cf68a744ff9be2..1db24a613a3f02d07476824a53b513a422ce289a 100644 (file)
@@ -1,4 +1,4 @@
-/*     $Id: tbl_data.c,v 1.30 2014/04/20 16:46:05 schwarze Exp $ */
+/*     $Id: tbl_data.c,v 1.31 2014/04/23 16:08:33 schwarze Exp $ */
 /*
  * Copyright (c) 2009, 2010, 2011 Kristaps Dzonsons <kristaps@bsd.lv>
  * Copyright (c) 2011 Ingo Schwarze <schwarze@openbsd.org>
@@ -167,8 +167,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);