mandoc strlcat

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

mandoc strlcat

Ted Unangst-6
I was looking at mandoc and noticed it has too many strlcats (a common
affliction affecting quite a few programs.) It's faster and simpler to
use snprintf.

The code in roff.c was doing something twisty with the length argument
to strlcpy. Doing fancy length tricks kind of defeats the purpose of
having a simple to use function like strlcpy. I believe the intention
was to only copy part of the string, so I have retained that feature.

Index: html.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/html.c,v
retrieving revision 1.30
diff -u -p -r1.30 html.c
--- html.c 28 May 2012 13:00:51 -0000 1.30
+++ html.c 23 May 2013 20:52:43 -0000
@@ -618,10 +618,7 @@ void
 bufcat_style(struct html *h, const char *key, const char *val)
 {
 
- bufcat(h, key);
- bufcat(h, ":");
- bufcat(h, val);
- bufcat(h, ";");
+ bufcat_fmt(h, "%s:%s;", key, val);
 }
 
 void
Index: man_html.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/man_html.c,v
retrieving revision 1.48
diff -u -p -r1.48 man_html.c
--- man_html.c 17 Nov 2012 00:25:20 -0000 1.48
+++ man_html.c 23 May 2013 20:50:52 -0000
@@ -301,9 +301,10 @@ man_root_pre(MAN_ARGS)
  struct tag *t, *tt;
  char b[BUFSIZ], title[BUFSIZ];
 
- b[0] = 0;
  if (man->vol)
- (void)strlcat(b, man->vol, BUFSIZ);
+ strlcpy(b, man->vol, sizeof(b));
+ else
+ b[0] = '\0';
 
  assert(man->title);
  assert(man->msec);
Index: mandocdb.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mandocdb.c,v
retrieving revision 1.42
diff -u -p -r1.42 mandocdb.c
--- mandocdb.c 24 May 2012 23:33:23 -0000 1.42
+++ mandocdb.c 23 May 2013 20:23:31 -0000
@@ -286,7 +286,6 @@ mandocdb(int argc, char *argv[])
  int ch, i, flags;
  DB *hash; /* temporary keyword hashtable */
  BTREEINFO info; /* btree configuration */
- size_t sz1, sz2;
  struct buf buf, /* keyword buffer */
  dbuf; /* description buffer */
  struct of *of; /* list of files for processing */
@@ -397,19 +396,12 @@ mandocdb(int argc, char *argv[])
  perror(dir);
  exit((int)MANDOCLEVEL_BADARG);
  }
- if (strlcat(pbuf, "/", PATH_MAX) >= PATH_MAX) {
- fprintf(stderr, "%s: path too long\n", pbuf);
- exit((int)MANDOCLEVEL_BADARG);
- }
-
- strlcat(mdb.dbn, pbuf, MAXPATHLEN);
- sz1 = strlcat(mdb.dbn, MANDOC_DB, MAXPATHLEN);
-
- strlcat(mdb.idxn, pbuf, MAXPATHLEN);
- sz2 = strlcat(mdb.idxn, MANDOC_IDX, MAXPATHLEN);
 
- if (sz1 >= MAXPATHLEN || sz2 >= MAXPATHLEN) {
- fprintf(stderr, "%s: path too long\n", mdb.idxn);
+ if ((size_t)snprintf(mdb.dbn, sizeof(mdb.dbn), "%s/%s", pbuf,
+    MANDOC_DB) >= sizeof(mdb.dbn) ||
+    (size_t)snprintf(mdb.idxn, sizeof(mdb.idxn), "%s/%s", pbuf,
+    MANDOC_IDX) >= sizeof(mdb.dbn)) {
+ fprintf(stderr, "%s: path too long\n", mdb.dbn);
  exit((int)MANDOCLEVEL_BADARG);
  }
 
@@ -486,8 +478,8 @@ mandocdb(int argc, char *argv[])
 
  flags = O_CREAT | O_EXCL | O_RDWR;
  while (NULL == mdb.db) {
- strlcpy(mdb.dbn, MANDOC_DB, MAXPATHLEN);
- strlcat(mdb.dbn, ".XXXXXXXXXX", MAXPATHLEN);
+ snprintf(mdb.dbn, sizeof(mdb.dbn), "%s.XXXXXXXXXX",
+    MANDOC_DB);
  if (NULL == mktemp(mdb.dbn)) {
  perror(mdb.dbn);
  exit((int)MANDOCLEVEL_SYSERR);
@@ -500,8 +492,8 @@ mandocdb(int argc, char *argv[])
  }
  }
  while (NULL == mdb.idx) {
- strlcpy(mdb.idxn, MANDOC_IDX, MAXPATHLEN);
- strlcat(mdb.idxn, ".XXXXXXXXXX", MAXPATHLEN);
+ snprintf(mdb.idxn, sizeof(mdb.idxn), "%s.XXXXXXXXXX",
+    MANDOC_IDX);
  if (NULL == mktemp(mdb.idxn)) {
  perror(mdb.idxn);
  unlink(mdb.dbn);
@@ -1809,12 +1801,8 @@ ofile_dirbuild(const char *dir, const ch
  continue;
  }
 
- buf[0] = '\0';
- strlcat(buf, dir, MAXPATHLEN);
- strlcat(buf, "/", MAXPATHLEN);
- sz = strlcat(buf, fn, MAXPATHLEN);
-
- if (MAXPATHLEN <= sz) {
+ if ((size_t)snprintf(buf, sizeof(buf), "%s/%s", dir,
+    fn) >= sizeof(buf)) {
  if (warnings) fprintf(stderr, "%s/%s: "
     "path too long\n", dir, fn);
  continue;
@@ -1882,10 +1870,8 @@ ofile_dirbuild(const char *dir, const ch
  * when they used to have source manuals before,
  * and in ports, old manuals get removed on update.
  */
- if (0 == use_all && MANDOC_FORM & src_form &&
- '\0' != *psec) {
- buf[0] = '\0';
- strlcat(buf, dir, MAXPATHLEN);
+ if (0 == use_all && MANDOC_FORM & src_form && '\0' != *psec) {
+ strlcpy(buf, dir, MAXPATHLEN);
  p = strrchr(buf, '/');
  if ('\0' != *parch && NULL != p)
  for (p--; p > buf; p--)
@@ -1920,12 +1906,11 @@ ofile_dirbuild(const char *dir, const ch
  }
  }
 
- buf[0] = '\0';
  assert('.' == dir[0]);
- if ('/' == dir[1]) {
- strlcat(buf, dir + 2, MAXPATHLEN);
- strlcat(buf, "/", MAXPATHLEN);
- }
+ if ('/' == dir[1])
+ snprintf(buf, sizeof(buf), "%s/", dir + 2);
+ else
+ buf[0] = '\0';
  sz = strlcat(buf, fn, MAXPATHLEN);
  if (sz >= MAXPATHLEN) {
  if (warnings) fprintf(stderr,
Index: mdoc_html.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mdoc_html.c,v
retrieving revision 1.67
diff -u -p -r1.67 mdoc_html.c
--- mdoc_html.c 17 Nov 2012 00:25:20 -0000 1.67
+++ mdoc_html.c 23 May 2013 20:33:25 -0000
@@ -525,15 +525,12 @@ mdoc_root_pre(MDOC_ARGS)
  struct tag *t, *tt;
  char b[BUFSIZ], title[BUFSIZ];
 
- strlcpy(b, meta->vol, BUFSIZ);
+ if (meta->arch)
+ snprintf(b, sizeof(b), "%s (%s)", meta->vol, meta->arch);
+ else
+ snprintf(b, sizeof(b), "%s", meta->vol);
 
- if (meta->arch) {
- strlcat(b, " (", BUFSIZ);
- strlcat(b, meta->arch, BUFSIZ);
- strlcat(b, ")", BUFSIZ);
- }
-
- snprintf(title, BUFSIZ - 1, "%s(%s)", meta->title, meta->msec);
+ snprintf(title, sizeof(title), "%s(%s)", meta->title, meta->msec);
 
  PAIR_SUMMARY_INIT(&tag[0], "Document Header");
  PAIR_CLASS_INIT(&tag[1], "head");
@@ -1016,8 +1013,7 @@ 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);
+ snprintf(buf, sizeof(buf), "list %s", lists[n->norm->Bl.type]);
  PAIR_INIT(&tag[1], ATTR_CLASS, buf);
 
  /* Set the block's left-hand margin. */
Index: mdoc_validate.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/mdoc_validate.c,v
retrieving revision 1.109
diff -u -p -r1.109 mdoc_validate.c
--- mdoc_validate.c 17 Nov 2012 00:25:20 -0000 1.109
+++ mdoc_validate.c 23 May 2013 20:32:42 -0000
@@ -1241,8 +1241,7 @@ post_at(POST_ARGS)
  q = mdoc->last->child->string;
  sz = strlen(p) + strlen(q) + 1;
  buf = mandoc_malloc(sz);
- strlcpy(buf, p, sz);
- strlcat(buf, q, sz);
+ snprintf(buf, sz, "%s%s", p, q);
  free(mdoc->last->child->string);
  mdoc->last->child->string = buf;
  }
@@ -2288,7 +2287,7 @@ post_os(POST_ARGS)
  return(1);
  }
 #ifdef OSNAME
- if (strlcat(buf, OSNAME, BUFSIZ) >= BUFSIZ) {
+ if (strlcpy(buf, OSNAME, sizeof(buf)) >= sizeof(buf)) {
  mdoc_nmsg(mdoc, n, MANDOCERR_MEM);
  return(0);
  }
@@ -2299,15 +2298,8 @@ post_os(POST_ARGS)
                         return(post_prol(mdoc));
                 }
 
- if (strlcat(buf, utsname.sysname, BUFSIZ) >= BUFSIZ) {
- mdoc_nmsg(mdoc, n, MANDOCERR_MEM);
- return(0);
- }
- if (strlcat(buf, " ", BUFSIZ) >= BUFSIZ) {
- mdoc_nmsg(mdoc, n, MANDOCERR_MEM);
- return(0);
- }
- if (strlcat(buf, utsname.release, BUFSIZ) >= BUFSIZ) {
+ if ((size_t)snprintf(buf, sizeof(buf), "%s %s",
+    utsname.sysname, utsname.release) >= sizeof(buf)) {
  mdoc_nmsg(mdoc, n, MANDOCERR_MEM);
  return(0);
  }
Index: roff.c
===================================================================
RCS file: /cvs/src/usr.bin/mandoc/roff.c,v
retrieving revision 1.48
diff -u -p -r1.48 roff.c
--- roff.c 7 Jul 2012 18:27:36 -0000 1.48
+++ roff.c 23 May 2013 21:02:02 -0000
@@ -18,6 +18,7 @@
 #include <assert.h>
 #include <ctype.h>
 #include <stdlib.h>
+#include <stdio.h>
 #include <string.h>
 
 #include "mandoc.h"
@@ -572,9 +573,8 @@ again:
  nsz = *szp + strlen(res) + 1;
  n = mandoc_malloc(nsz);
 
- strlcpy(n, *bufp, (size_t)(stesc - *bufp + 1));
- strlcat(n, res, nsz);
- strlcat(n, cp + (maxl ? 0 : 1), nsz);
+ snprintf(n, nsz, "%.*s%s%s", (int)(stesc - *bufp + 1),
+    *bufp, res, cp + (maxl ? 0 : 1));
 
  free(*bufp);
 
@@ -1567,9 +1567,8 @@ roff_userdef(ROFF_ARGS)
  *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);
+ snprintf(n2, *szp, "%.*s%s%s", (int)(cp - n1 + 1),
+    n1, arg[i], cp + 3);
 
  cp = n2 + (cp - n1);
  free(n1);

Reply | Threaded
Open this post in threaded view
|

Re: mandoc strlcat

William Ahern-2
On Thu, May 23, 2013 at 05:05:45PM -0400, Ted Unangst wrote:
> I was looking at mandoc and noticed it has too many strlcats (a common
> affliction affecting quite a few programs.) It's faster and simpler to
> use snprintf.

In glibc snprintf has a memory allocation failure mode. I'm curious: is
OpenBSD committed to avoiding extensions (locale features, etc) which might
trigger allocation failure?

Reply | Threaded
Open this post in threaded view
|

Re: mandoc strlcat

Theo de Raadt
In reply to this post by Ted Unangst-6
> On Thu, May 23, 2013 at 05:05:45PM -0400, Ted Unangst wrote:
> > I was looking at mandoc and noticed it has too many strlcats (a common
> > affliction affecting quite a few programs.) It's faster and simpler to
> > use snprintf.
>
> In glibc snprintf has a memory allocation failure mode.

In OpenBSD, snprintf is designed to be thread and signal-handler safe,
as long as you don't use certain dangerous features.  I'm afraid I
can't find documentation which defines which are dangerous or not, but
remember auditing our tree to improve the situation.

> I'm curious: is
> OpenBSD committed to avoiding extensions (locale features, etc) which might
> trigger allocation failure?

I don't know if we are commited to such a restriction.  We could add such
things, but then put them in the "dangerous" catagory, to not be used in
unsafe situations...

Hmm, where are our docs for that...

Reply | Threaded
Open this post in threaded view
|

Re: mandoc strlcat

Ted Unangst-6
On Thu, May 23, 2013 at 21:38, Theo de Raadt wrote:
>> In glibc snprintf has a memory allocation failure mode.

>> I'm curious: is
>> OpenBSD committed to avoiding extensions (locale features, etc) which might
>> trigger allocation failure?

Yes. I mean, what in the world is snprintf doing allocating some
locale crap to implement a behavior that strlcat clearly doesn't need
to allocate memory for?

>
> I don't know if we are commited to such a restriction.  We could add such
> things, but then put them in the "dangerous" catagory, to not be used in
> unsafe situations...
>
> Hmm, where are our docs for that...

It's in man signal.

The only thing you can't use is floating point, because dtoa is crazy,
but I think it'd even be possible to pass the buffer in from vfprintf
and make that signal safe too. Just nobody cares.

Reply | Threaded
Open this post in threaded view
|

Re: mandoc strlcat

Theo de Raadt
In reply to this post by Ted Unangst-6
> It's in man signal.
>
> The only thing you can't use is floating point, because dtoa is crazy,

The *5 table, yes.

I tried to improve the situation there.  I nearly lost my mind.

Reply | Threaded
Open this post in threaded view
|

Re: mandoc strlcat

Marc Espie-2
In reply to this post by Theo de Raadt
On Thu, May 23, 2013 at 09:38:57PM -0600, Theo de Raadt wrote:

> > On Thu, May 23, 2013 at 05:05:45PM -0400, Ted Unangst wrote:
> > > I was looking at mandoc and noticed it has too many strlcats (a common
> > > affliction affecting quite a few programs.) It's faster and simpler to
> > > use snprintf.
> >
> > In glibc snprintf has a memory allocation failure mode.
>
> In OpenBSD, snprintf is designed to be thread and signal-handler safe,
> as long as you don't use certain dangerous features.  I'm afraid I
> can't find documentation which defines which are dangerous or not, but
> remember auditing our tree to improve the situation.

From what I remember, the unsafe extensions were the $# positional
markers, that allow one to swap arguments around.

Reply | Threaded
Open this post in threaded view
|

Re: mandoc strlcat

Mark Kettenis
In reply to this post by Theo de Raadt
> From: Theo de Raadt <[hidden email]>
> Date: Thu, 23 May 2013 21:38:57 -0600
>
> > On Thu, May 23, 2013 at 05:05:45PM -0400, Ted Unangst wrote:
> > > I was looking at mandoc and noticed it has too many strlcats (a common
> > > affliction affecting quite a few programs.) It's faster and simpler to
> > > use snprintf.
> >
> > In glibc snprintf has a memory allocation failure mode.
>
> In OpenBSD, snprintf is designed to be thread and signal-handler safe,
> as long as you don't use certain dangerous features.  I'm afraid I
> can't find documentation which defines which are dangerous or not, but
> remember auditing our tree to improve the situation.

But the reason we did this was to reduce the amount of damage badly
written signal handlers could do.  Not to encourage people to actually
use the *printf(3) family of functions in signal handlers.

Reply | Threaded
Open this post in threaded view
|

Re: mandoc strlcat

Theo de Raadt
In reply to this post by Ted Unangst-6
> But the reason we did this was to reduce the amount of damage badly
> written signal handlers could do.  Not to encourage people to actually
> use the *printf(3) family of functions in signal handlers.

Well... we had to use something..