Re: mandoc strlcat
> 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..
Re: mandoc strlcat
> From: Theo de Raadt > 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.
Re: mandoc strlcat
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.
Re: mandoc strlcat
> 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.
Re: mandoc strlcat
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.
Re: mandoc strlcat
> 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...
Re: mandoc strlcat
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?
mandoc strlcat
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 - 1.30
+++ html.c 23 May 2013 20:52:43 -
@@ -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 - 1.48
+++ man_html.c 23 May 2013 20:50:52 -
@@ -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 - 1.42
+++ mandocdb.c 23 May 2013 20:23:31 -
@@ -286,7 +286,6 @@ mandocdb(int argc, char *argv[])
int ch, i, flags;
DB *hash; /* temporary keyword hashtable */
BTREEINFOinfo; /* 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, ".XX", MAXPATHLEN);
+ snprintf(mdb.dbn, sizeof(mdb.dbn), "%s.XX",
+ 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, ".XX", MAXPATHLEN);
+ snprintf(mdb.idxn, sizeof(mdb.idxn), "%s.XX",
+ 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) {
+
