This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][BZ #14771] Fortify tweak for snprintf et al.


On Sun, Oct 20, 2013 at 12:35:31PM +0200, Florian Weimer wrote:
> On 10/20/2013 04:47 AM, Rich Felker wrote:
> >On Fri, Oct 18, 2013 at 01:30:07PM +0200, Florian Weimer wrote:
> >>+#ifndef _FORTIFY_H
> >>+#define _FORTIFY_H 1
> >>+
> >>+#include <limits.h>
> >>+
> >>+__BEGIN_DECLS
> >>+
> >>+/* Check that USER length does not exceed the COMPILER length (which
> >>+   should have been obtained through __builtin_object_size), and that
> >>+   USER does not exceed INT_MAX minus some reserve.
> >>+
> >>+   Technically, a caller to functions like snprintf could specify
> >>+   (size_t)-1 as the buffer length to get the behavior of sprintf
> >>+   (without target buffer length checking), but we do not support this
> >>+   use in fortify mode.  */
> >>+static inline void
> >>+__check_buffer_length_int_max (size_t user, size_t compiler)
> >>+{
> >>+  if (__builtin_expect (compiler < user || user >= INT_MAX - 1024, 0))
> >>+    __chk_fail ();
> >>+}
> >
> >POSIX already requires an error from snprintf if the size passed to
> >snprintf is greater than INT_MAX, although this possibly conflicts
> >with the requirements of ISO C (this needs to be addressed by the ISO
> >C committee). If you believe there is any value in rejecting sizes
> >above INT_MAX-1024, I think you need a strong argument for doing so.
> 
> See the patch from Exim I posted.
> 
> Note that this is for fortify mode only.  Both ISO C and POSIX
> require that you end up with sprintf behavior when you specify
> (size_t)-1 as the buffer length.

No, they don't. POSIX explicitly requires an error if you pass
(size_t)-1 or any value larger than INT_MAX as the size argument.

> (Neither standard requires that
> the size argument is the length of the buffer.)  The fortify
> implementation already aborts in that case (even if the result would
> fit into the supplied buffer), so my proposal is less radical than
> you might think.

Then this should be fixed. It's a bug in fortify unless the point of
fortify is to make applications crash instead of getting back an error
return on the basis that you expect they don't check for the error
return.

> I'm open to tweaking the magic number, perhaps to ((size_t)-1)/2 -
> 1024 or something like that.  But I do think that check makes sense,
> as exemplified by Exim.

INT_MAX is always less than ((size_t)-1)/2 - 1024, so the check would
be completely redundant with the check against INT_MAX.

Rich


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]