[prev in list] [next in list] [prev in thread] [next in thread]
List: inn-workers
Subject: Re: The semantics of strlcpy and strlcat
From: Paul Eggert <eggert () cs ! ucla ! edu>
Date: 2016-01-23 11:24:28
Message-ID: 56A362EC.2000608 () cs ! ucla ! edu
[Download RAW message or body]
[This message is responding to
<https://sourceware.org/ml/libc-alpha/2016-01/msg00573.html>. I am removing
libc-alpha from CC:, and adding inn-workers to CC:, since this message is
INN-specific rather than being a glibc thing.]
Russ Allbery wrote:
> I'd like to remove them all from INN as well. Unfortunately, INN is a
> very old code base that's riddled with buffers of fixed size allocated off
> the stack with unclear object lifetimes and no further thought given to
> any form of memory management, which makes it hard to take the approach
> that I'm using with all other software I maintain (switching to asprintf).
> Short of a grand removal of static buffers that's an immense amount of
> work and may not happen for software that's mostly in maintenance mode,
> I'm not sure what the best solution is. (I stand by my original feeling
> that silent truncation is generally better than silent buffer overflows,
> but that's a little like saying that tuberculosis is better than ebola.)
Looking at INN it appears that strlcpy and strlcat are used as
belt-and-suspenders copiers -- callers do not check for truncation (except in
one place, where a caller checks for truncation incorrectly and this apparently
can lead to buffer overflow -- I just now filed a bug report about that to
inn-workers).
If so, how about the attached patch? It assumes the fix I sent earlier for the
truncation bug. The basic idea is that truncations should never occur, but if
any does occur then it should be caught and logged. I haven't tested this.
["inn1.diff" (text/x-diff)]
Index: include/clibrary.h
===================================================================
--- include/clibrary.h (revision 9981)
+++ include/clibrary.h (working copy)
@@ -202,6 +202,20 @@
extern size_t strlcpy(char *, const char *, size_t);
#endif
+/*
+ * Replace strlcat and strlcpy with a substitute implementation that
+ * logs truncation and does not return anything. At some point we
+ * should stop using these functions entirely, but in the meantime at
+ * least we can log when unexpected truncation returns.
+ */
+
+extern void xstrlcat(char *, const char *, size_t, const char *, int);
+extern void xstrlcpy(char *, const char *, size_t, const char *, int);
+#undef strlcat
+#undef strlcpy
+#define strlcat(dst, src, size) xstrlcat(dst, src, size, __FILE__, __LINE__)
+#define strlcpy(dst, src, size) xstrlcpy(dst, src, size, __FILE__, __LINE__)
+
END_DECLS
#endif /* !CLIBRARY_H */
Index: lib/Makefile
===================================================================
--- lib/Makefile (revision 9981)
+++ lib/Makefile (working copy)
@@ -24,7 +24,7 @@
radix32.c readin.c \
remopen.c reservedfd.c resource.c sendarticle.c sendpass.c \
sequence.c timer.c tst.c uwildmat.c vector.c wire.c \
- xfopena.c xmalloc.c xsignal.c xwrite.c
+ xfopena.c xmalloc.c xsignal.c xstrlcat.c xstrlcpy.c xwrite.c
# Sources for additional functions only built to replace missing system ones.
EXTRA_SOURCES = alloca.c asprintf.c fseeko.c ftello.c getaddrinfo.c \
Index: lib/strlcat.c
===================================================================
--- lib/strlcat.c (revision 9981)
+++ lib/strlcat.c (working copy)
@@ -27,12 +27,13 @@
#include "config.h"
#include "clibrary.h"
+#undef strlcat
+
/*
* If we're running the test suite, rename strlcat to avoid conflicts with
* the system version.
*/
#if TESTING
-# undef strlcat
# define strlcat test_strlcat
size_t test_strlcat(char *, const char *, size_t);
#endif
Index: lib/strlcpy.c
===================================================================
--- lib/strlcpy.c (revision 9981)
+++ lib/strlcpy.c (working copy)
@@ -26,12 +26,13 @@
#include "config.h"
#include "clibrary.h"
+#undef strlcpy
+
/*
* If we're running the test suite, rename strlcpy to avoid conflicts with
* the system version.
*/
#if TESTING
-# undef strlcpy
# define strlcpy test_strlcpy
size_t test_strlcpy(char *, const char *, size_t);
#endif
Index: lib/xstrlcat.c
===================================================================
--- lib/xstrlcat.c (nonexistent)
+++ lib/xstrlcat.c (working copy)
@@ -0,0 +1,29 @@
+/* $Id$
+ *
+ * strlcat with logging of unexpected truncation and returning void.
+ *
+ * Written by Paul Eggert <eggert@cs.ucla.edu>
+ *
+ * The authors hereby relinquish any claim to any copyright that they may have
+ * in this work, whether granted under contract or by operation of law or
+ * international treaty, and hereby commit to the public, at large, that they
+ * shall not, at any time in the future, seek to enforce any copyright in this
+ * work against any person or entity, or prevent any person or entity from
+ * copying, publishing, distributing or creating derivative works of this
+ * work.
+ */
+
+#include "config.h"
+#include "clibrary.h"
+
+#include "inn/xmalloc.h"
+
+void
+xstrlcat(char *dst, const char *src, size_t size, const char *file, int line)
+{
+ size_t srclen = strlen(src);
+ char *dstend = memchr(dst, 0, size);
+ if (!dstend || size - (dstend - dst) <= srclen)
+ (*xmalloc_error_handler)("strlcat", size, file, line);
+ (strlcat)(dst, src, size);
+}
Index: lib/xstrlcpy.c
===================================================================
--- lib/xstrlcpy.c (nonexistent)
+++ lib/xstrlcpy.c (working copy)
@@ -0,0 +1,27 @@
+/* $Id$
+ *
+ * strlcpy with logging of unexpected truncation and returning void.
+ *
+ * Written by Paul Eggert <eggert@cs.ucla.edu>
+ *
+ * The authors hereby relinquish any claim to any copyright that they may have
+ * in this work, whether granted under contract or by operation of law or
+ * international treaty, and hereby commit to the public, at large, that they
+ * shall not, at any time in the future, seek to enforce any copyright in this
+ * work against any person or entity, or prevent any person or entity from
+ * copying, publishing, distributing or creating derivative works of this
+ * work.
+ */
+
+#include "config.h"
+#include "clibrary.h"
+
+#include "inn/xmalloc.h"
+
+void
+xstrlcpy(char *dst, const char *src, size_t size, const char *file, int line)
+{
+ if (size <= strlen(src))
+ (*xmalloc_error_handler)("strlcpy", size, file, line);
+ (strlcpy)(dst, src, size);
+}
_______________________________________________
inn-workers mailing list
inn-workers@lists.isc.org
https://lists.isc.org/mailman/listinfo/inn-workers
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic