On Wed, Apr 13, 2016 at 09:48:26PM +0200, Vincent Lefevre wrote: > On 2016-04-13 07:24:50 -0700, Kevin J. McCarthy wrote: > > On Wed, Apr 13, 2016 at 10:34:31AM +0200, Vincent Lefevre wrote: > > > In this second patch: > > > > > > + pad = ((signed)(destlen - wlen - len)) / pl; > > > > > > The cast to "signed" seems useless and confusing: what is its goal? > > > > Thanks for taking a look at the patches. That particular line was > > actually just moved from above the "if (pad >= 0)" to inside it. > > > > I think because destlen, wlen, and len are all size_t, if wlen+len were > > larger then destlen the resulting subtraction could turn into a large > > value, instead of the desired negative number (e.g., if we ran out of > > space). > > But in this case, the result of the cast is implementation-defined; > in particular, it could yield a signal (I hope that there will be > such an option in GCC one day, for security, because this may be the > consequence of an unexpected integer overflow, which could become a > buffer overflow). So, the code is incorrect.
Hi Vincent,
I confess I don't follow where the problem is (due to my own ignorance).
Is casting a size_t to a (signed) implementation defined or undefined?
Is it a problem to then divide that by an int and assign the result to
an int?
Are we forced to write something like:
/* try to consume as many columns as we can, if we don't have
* memory for that, use as much memory as possible */
if (wlen + (pad * pl) + len > destlen)
if (wlen + len > destlen)
pad = 0;
else
pad = (destlen - wlen - len) / pl;
else
> And the division with a negative argument is only defined in C99,
> so that may be another problem (it is known that some compilers may
> still deviate from C99, including GCC unless -std=c99 is provided).
Fortunately, we do have AC_PROG_CC_C99 in our configure.ac now.
--
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
http://www.8t8.us/configs/gpg-key-transition-statement.txt
signature.asc
Description: PGP signature
