Re: [Bug-gnulib] linebreak.c proposed patches for size-calculation overflows

[Bruno Haible]

Paul Eggert wrote:
> > size_t overflow checking is actually a different facility than
> > xmalloc(), therefore I think it deserves its own .h file.
>
> Makes sense, but I suspect it's not limited to size_t.

All the cases I've seen up to now deal with size_t, because the purpose
is to avoid crashes after malloc(). If later we see that we need the
facility for other types as well, we can extend it.

> > > +      if (n <= (size_t)(-1) / (sizeof (size_t) + 2 * MB_LEN_MAX)
> >
> > This way is not the right path: the code becomes ununderstandable.
> > (In this case, I even doubt the correctness: why MB_LEN_MAX? MB_LEN_MAX
> > is 6 in glibc, but iconv() can also apply to encodings, like
> > ISO-2022-JP-1, where a single character can lead to 8 bytes or more.)
>
> You can substitute whatever value that you like for MB_LEN_MAX; if the
> proper value is 8, it should be written down somewhere, and you can
> use that. ... certainly more efficient.

It may be 8 or 12 for glibc, or maybe no upper bound exists for other
iconv() implementations.

Efficiency is not the issue here: we are trying to make code that can
be proven to be correct and overflow-free. If while doing that we
introduce suspicious dependencies and fallacious reasonings about iconv(),
the code is not provably correct any more. And if furthermore the code
is platform dependent, it will go unnoticed while porting to a new platform,
resulting in incorrect code that will fail to do what it was intended for.

> > #ifndef SIZE_MAX
> > # define SIZE_MAX ((size_t) -1)
> > #endif
>
> xsize.h shouldn't define SIZE_MAX, as various standard headers define
> it to a better value (one that can be used in #if), and the better
> value will collide with this macro definition.

OK, I can #include <stdint.h> as well.

> We could define a differently-named macro, SIZE_MAXIMUM, say.

This would be against our general strategy for gapping missing definitions
on same platforms. That general strategy is to use the same name as the
standards use, in the expectation that in the long run the deficient
systems will disappear.

> > /* Sum of two sizes, with overflow check.  */
> > static inline size_t
> > xsum (size_t size1, size_t size2)
> > {
> >   size_t sum = size1 + size2;
> >   return (sum >= size1 ? sum : SIZE_MAX);
> > }
>
> Shouldn't this be a macro, for the same reason that xtimes is a macro?

I've seen no occurrence when one would need to pass something bigger
than a size_t. And when one needs to do that, a simple conversion macro
will do it as well.

  #define xcast_size_t(N) ((N) <= SIZE_MAX ? (size_t)(N) : SIZE_MAX)

> > /* Sum of three sizes, with overflow check.  */
> > static inline size_t
> > xsum3 (size_t size1, size_t size2, size_t size3)
>
> I dunno; this looks a bit overkill to me; also, it's less efficient
> than doing it by hand.

I don't care about efficiency in an area where one wants to avoid core
dumps reliably. Maintainability is more important to me here. If the
code is too clever, the next maintainer will break it.

> > /* Check for overflow.  */
> > #define SIZE_OVERFLOW_P(SIZE) \
> >   ((SIZE) == SIZE_MAX)
>
> I'd rather use a lower-case name beginning with 'x', for consistency.

OK. Thanks for your feedback.

Bruno