[patch] fixed signed int overflow (in move.c)

[Dominique Pellé]

Hi

Here are more signed int overflows with undefined behavior
discovered with the IOC tool (http://embed.cs.utah.edu/ioc/):

CLANG ARITHMETIC UNDEFINED at <move.c, (2591:12)> : Op: +, Reason :
Signed Addition Overflow, BINARY OPERATION: left (int32): 2147483647
right (int32): 1

CLANG ARITHMETIC UNDEFINED at <move.c, (2603:12)> : Op: +, Reason :
Signed Addition Overflow, BINARY OPERATION: left (int32): 2147483647
right (int32): 2147483647

CLANG ARITHMETIC UNDEFINED at <move.c, (2603:41)> : Op: +, Reason :
Signed Addition Overflow, BINARY OPERATION: left (int32): 2147483647
right (int32): 1

I can reproduce these overflows as follows:

$ yes 1 | head -5 > 1
$ yes 2 | head -5 > 2
$ vim -u NONE -c 'set wrap' -d 1 2

Then press <PgDown> followed by <PgUp> and the overflow happens.

Even assuming a two's complement representation of
signed value, I think that code is still wrong here:

move.c:

2591 if (h3 + h2 > min_height)
2592 {
2593 *lp = loff0; /* no overlap */
2594 return;
2595 }

h3 and/or h2 are signed int variables. They can be
equal to MAXCOL (0x7fffffffL). So the addition
at line 2591 can overflow giving in general a negative
value (but in theory behavior is undefined for signed
int overflows). The intention of MAXCOL here was
behave as a large height.

Attached patch fixes it but please review it.

IOC tool no longer complains with the patch.

Regards
-- Dominique

[Christ van Willegen]

Hello Dominique,

On Mon, Nov 5, 2012 at 8:40 PM, Dominique Pellé
<dominiq...@gmail.com> wrote:

> Attached patch fixes it but please review it.

This part of the diff looks off to me:

- if (h4 + h3 + h2 > min_height || h3 + h2 + h1 > min_height)
+ if (h4 == MAXCOL + h4 + h3 + h2 > min_height || h3 + h2 + h1 > min_height)

You probably meant 'h4 == MAXCOL || h4 + h3 + h2 > min_height' here?

Christ van Willegen
--
09 F9 11 02 9D 74 E3 5B D8 41 56 C5 63 56 88 C0

[Dominique Pellé]

Christ van Willegen wrote:

> Hello Dominique,
>
> On Mon, Nov 5, 2012 at 8:40 PM, Dominique Pellé
> <dominiq...@gmail.com> wrote:
>
>> Attached patch fixes it but please review it.
>
> This part of the diff looks off to me:
>
> - if (h4 + h3 + h2 > min_height || h3 + h2 + h1 > min_height)
> + if (h4 == MAXCOL + h4 + h3 + h2 > min_height || h3 + h2 + h1 > min_height)
>
> You probably meant 'h4 == MAXCOL || h4 + h3 + h2 > min_height' here?
>
> Christ van Willegen

Thanks for spotting it. You are of course right.
Attached is the updated patch.

-- Dominique

[Bram Moolenaar]

Thanks, I'll add this to the todo list.

--
Shift happens.
-- Doppler

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[Bram Moolenaar]

[Bram Moolenaar]