Errors found by clang UBSan

[Yegappan Lakshmanan]

Hi all,

When running the Vim tests with UBSan, the following errors are
reported:

channel.c:713:5: runtime error: load of misaligned address
0x60b0000069ca for type 'char *', which requires 8 byte alignment
0x60b0000069ca: note: pointer points here

eval.c:4432:10: runtime error: negation of -9223372036854775808 cannot
be represented in type 'varnumber_T' (aka 'long long'); cast to an
unsigned type to negate this value to itself

- Yegappan

[Dominique Pellé]

Yegappan Lakshmanan wrote:

> Hi all,
>
> When running the Vim tests with UBSan, the following errors are
> reported:
>
> channel.c:713:5: runtime error: load of misaligned address
> 0x60b0000069ca for type 'char *', which requires 8 byte alignment
> 0x60b0000069ca: note: pointer points here

Line 713 is is this for me:

713 memcpy((char *)&server.sin_addr, host->h_addr, host->h_length);

I don't see this error with ubsan. A char* should not require 8 byte
alignment anyway. Strange.

> eval.c:4432:10: runtime error: negation of -9223372036854775808 cannot
> be represented in type 'varnumber_T' (aka 'long long'); cast to an
> unsigned type to negate this value to itself

I see it too.
It's coming from this line in Test_num64() in test_viml.vim:

call assert_equal(-9223372036854775808, 0 / 0)

Running :echo -9223372036854775808
also gives the ubsan error.

viml parser is considering the number 9223372036854775808
(which is not a valid long long) and then negating it, instead
of considering it as number -9223372036854775808
(which is a valid long long). Not sure how it can be fix.
Even though it's undefined behavior, it probably works
on all platforms in practice. The test would probably
fail if it did not work. But it would be good to avoid undefined
behavior.

Dominique

[Mike Williams]

On 14/12/2016 08:44, Dominique Pellé wrote:

> Yegappan Lakshmanan wrote:
>> eval.c:4432:10: runtime error: negation of -9223372036854775808 cannot
>> be represented in type 'varnumber_T' (aka 'long long'); cast to an
>> unsigned type to negate this value to itself
>
> I see it too.
> It's coming from this line in Test_num64() in test_viml.vim:
>
> call assert_equal(-9223372036854775808, 0 / 0)
>
> Running :echo -9223372036854775808
> also gives the ubsan error.
>
> viml parser is considering the number 9223372036854775808
> (which is not a valid long long) and then negating it, instead
> of considering it as number -9223372036854775808
> (which is a valid long long). Not sure how it can be fix.
> Even though it's undefined behavior, it probably works
> on all platforms in practice. The test would probably
> fail if it did not work. But it would be good to avoid undefined
> behavior.

The usual idiom for writing an integer constant for the largest negative
value 2s complement integer is to negate the largest positive value and
subtract 1. So use (-9223372036854775807 - 1)?

Mike
--
Don't play with matches. Lighters work much better.

[Yegappan Lakshmanan]

Hi,

On Wed, Dec 14, 2016 at 12:44 AM, Dominique Pellé
<dominiq...@gmail.com> wrote:
> Yegappan Lakshmanan wrote:
>
>> Hi all,
>>
>> When running the Vim tests with UBSan, the following errors are
>> reported:
>>
>> channel.c:713:5: runtime error: load of misaligned address
>> 0x60b0000069ca for type 'char *', which requires 8 byte alignment
>> 0x60b0000069ca: note: pointer points here
>
> Line 713 is is this for me:
>

Yes. I forgot to include the relevant code information in my e-mail.

>
> 713 memcpy((char *)&server.sin_addr, host->h_addr, host->h_length);
>
> I don't see this error with ubsan. A char* should not require 8 byte
> alignment anyway. Strange.
>

I am building Vim (8.0.133) on MacOS with the following sanitizer flags:

SANITIZER_CFLAGS = -g -O0 -fsanitize=address,undefined -fno-omit-frame-pointer

I am using the tools from the latest Xcode 8.2.

I am not sure how to fix these errors though.

Regards,
Yegappan

[Mike Williams]

Apologies. That just solves the issue with the test and not the
underlying problem. Caffeine took too long to kick in.

The cause is a combination of things. eval7() parses integers as
unsigned values but uses the signed value returned from vim_str2nr()
before applying any unary minus. In turn vim_str2nr() casts the
unsigned value of the integer to a signed value, so the value it returns
depends on whether the top bit is set from the unsigned calculation.
(Casting is lying to the compiler who will have its revenge one day -
looks like today is that day ;))

VIM doesn't signal (AFAIK) out of range integer values. At the moment,
with the wrapping unsigned integer math in vim_str2nr(), the result for
large integers is unpredictable (for those not concerned with integer
representation) Applying a saturating calculation would at least return
results as close as possible to the original integer value maintaining sign.

So the simplest change would be to have vim_str2nr() saturate and return
9223372036854775807 as the largest positive varnumber_T supported.
eval7() would then apply the unary minus and return -9223372036854775807

You will still need to use -9223372036854775807 - 1 in the original test
to generate the largest negative integer value possible in VIM.

TTFN

Mike
--
If opportunity doesn't knock, build a door.

[Mike Williams]

Patch attached to limit range of parsed integer numbers, cope with 2s
complement asymmetry, and modify test that was triggering the UB
warning. Tests pass but I don't have clang to check the UB warning so
please test for that.

Mike
--
I couldn't repair your brakes, so I made your horn louder.

[Bram Moolenaar]

Mike Williams wrote:

> On 14/12/2016 18:12, Mike Williams wrote:
> > On 14/12/2016 11:16, Mike Williams wrote:

> > results as close as possible to the original integer value maintaining si=

> gn.
> >
> > So the simplest change would be to have vim_str2nr() saturate and return
> > 9223372036854775807 as the largest positive varnumber_T supported.
> > eval7() would then apply the unary minus and return -9223372036854775807
> >
> > You will still need to use -9223372036854775807 - 1 in the original test
> > to generate the largest negative integer value possible in VIM.
>
> Patch attached to limit range of parsed integer numbers, cope with 2s
> complement asymmetry, and modify test that was triggering the UB
> warning. Tests pass but I don't have clang to check the UB warning so
> please test for that.

Thanks! I'll await confirmation it does fix the warnings.

--
"Microsoft is like Coke. It's a secret formula, all the money is from
distribution, and their goal is to get Coke everywhere. Open source is like
selling water. There are water companies like Perrier and Poland Spring, but
you're competing with something that's free." -- Carl Howe


/// 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 ///

[Yegappan Lakshmanan]

Hi,

I reran the tests with your patch (with UBSan) and the above runtime
error is resolved.

Only the following runtime errors are remaining:

eval.c:4068:15: runtime error: division by zero

channel.c:713:5: runtime error: load of misaligned address
0x60b0000069ca for type 'char *', which requires 8 byte alignment

- Yegappan

[Dominique Pellé]

Mike Williams wrote:

> Tests pass but I don't have clang to check the UB warning so please test for
> that.

Not sure which compiler you use, but gcc also has ubsan, if you use a version
recent enough. I don't remember in which gcc version ubsan was introduced,
probably gcc-4.8 or newer.

To use it, uncomment this line in vim/src/Makefile and rebuild:

#SANITIZER_CFLAGS = -g -O0 -fsanitize=undefined -fno-omit-frame-pointer

Regards
Dominique

[Mike Williams]

Hi

Good to know, but alas for "reasons" my most up to date version of gcc
is 4.6.3. Seems I could get clang 3.4 - a holidays project maybe.

TTFN

Mike
--
Lactomangulation: Abusing the "open here" spout on a milk carton.

[Yegappan Lakshmanan]

Hi,

This doesn't work on MacOS with Xcode 8.2. The following flags work:

SANITIZER_CFLAGS = -g -O0 -fsanitize=address,undefined -fno-omit-frame-pointer

- Yegappan

[Dominique Pellé]

Strange. I'd expect this to turn on both:
- asan (address sanitizer)
- and ubsan (undefined behavior sanitizer)

Dominique

[Yegappan Lakshmanan]

Hi,

On Sat, Dec 17, 2016 at 12:27 AM, Dominique Pellé

Yes. It turns on both the sanitizers. But I am not able to enable only
ubsan in the latest xcode 8.2 version. When I enable only ubsan,
I get the following linker error:

ld: file not found:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/8.0.0/lib/darwin/libclang_rt.ubsan_osx_dynamic.dylib

- Yegappan

[Mike Williams]

On 14/12/2016 08:44, Dominique Pellé wrote:
> Yegappan Lakshmanan wrote:

>> When running the Vim tests with UBSan, the following errors are
>> reported:
>>
>> channel.c:713:5: runtime error: load of misaligned address
>> 0x60b0000069ca for type 'char *', which requires 8 byte alignment
>> 0x60b0000069ca: note: pointer points here
>
> Line 713 is is this for me:
>
> 713 memcpy((char *)&server.sin_addr, host->h_addr, host->h_length);
>
> I don't see this error with ubsan. A char* should not require 8 byte
> alignment anyway. Strange.

This is a strange one. I would guess at a compiler issue. Stack
overflow coughs up these somewhat related issues on OSX which I think
you are using:

http://stackoverflow.com/questions/18821491/how-to-fix-unaligned-pointer-char#18822141
http://stackoverflow.com/questions/19916762/how-to-fix-unaligned-pointer-char?noredirect=1&lq=1

h_addr is usually a macro to h_addr_list[0] where h_addr_list is a
char**. If you aren't seeing a problem it may be a false positive.

HTH - TTFN

Mike
--
As confused as a moth in a lamp store!

[Yegappan Lakshmanan]

Hi,

The attached patch fixes this runtime error. But I am not sure whether
this is worth fixing it or not.

- Yegappan

[James McCoy]

On Fri, Dec 16, 2016 at 10:51:06AM +0000, Mike Williams wrote:
> Patch attached to limit range of parsed integer numbers, cope with 2s
> complement asymmetry, and modify test that was triggering the UB warning.
> Tests pass but I don't have clang to check the UB warning so please test for
> that.

I found a few more places that exhibit UB.

:echo float2nr(pow(2, 33)) * float2nr(pow(2, 33))
eval.c:4085:12: runtime error: signed integer overflow: 8589934592 * 8589934592 cannot be represented in type 'long'
#0 0x6c22e1 in eval6 /home/jamessan/src/github.com/vim/src/eval.c:4085:12
#1 0x6bf7d9 in eval5 /home/jamessan/src/github.com/vim/src/eval.c:3793:9
#2 0x6badf1 in eval4 /home/jamessan/src/github.com/vim/src/eval.c:3492:9
#3 0x6ba413 in eval3 /home/jamessan/src/github.com/vim/src/eval.c:3409:9
#4 0x66bcc3 in eval2 /home/jamessan/src/github.com/vim/src/eval.c:3341:9
#5 0x652332 in eval1 /home/jamessan/src/github.com/vim/src/eval.c:3269:9
#6 0x69703c in ex_echo /home/jamessan/src/github.com/vim/src/eval.c:8189:6
#7 0x84e8c1 in do_one_cmd /home/jamessan/src/github.com/vim/src/ex_docmd.c:2961:2
#8 0x82e1ef in do_cmdline /home/jamessan/src/github.com/vim/src/ex_docmd.c:1110:17
#9 0xcd9780 in nv_colon /home/jamessan/src/github.com/vim/src/normal.c:5398:15
#10 0xc7ef08 in normal_cmd /home/jamessan/src/github.com/vim/src/normal.c:1149:5
#11 0x14fa53b in main_loop /home/jamessan/src/github.com/vim/src/main.c:1311:6
#12 0x14f191e in vim_main2 /home/jamessan/src/github.com/vim/src/main.c:877:5
#13 0x14e3529 in main /home/jamessan/src/github.com/vim/src/main.c:415:12
#14 0x7f2eddf072b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
#15 0x41d8a9 in _start (/home/jamessan/src/github.com/vim/src/vim+0x41d8a9)

SUMMARY: AddressSanitizer: undefined-behavior eval.c:4085:12 in


:echo float2nr(pow(2, 63))
evalfunc.c:3341:29: runtime error: value 9.22337e+18 is outside the range of representable values of type 'long'
#0 0x6edfd9 in f_float2nr /home/jamessan/src/github.com/vim/src/evalfunc.c:3341:29
#1 0x6cd359 in call_internal_func /home/jamessan/src/github.com/vim/src/evalfunc.c:1001:5
#2 0x13a52b2 in call_func /home/jamessan/src/github.com/vim/src/userfunc.c:1372:14
#3 0x13a283b in get_func_tv /home/jamessan/src/github.com/vim/src/userfunc.c:455:8
#4 0x6c550c in eval7 /home/jamessan/src/github.com/vim/src/eval.c:4349:13
#5 0x6c1605 in eval6 /home/jamessan/src/github.com/vim/src/eval.c:3977:9
#6 0x6bf7d9 in eval5 /home/jamessan/src/github.com/vim/src/eval.c:3793:9
#7 0x6badf1 in eval4 /home/jamessan/src/github.com/vim/src/eval.c:3492:9
#8 0x6ba413 in eval3 /home/jamessan/src/github.com/vim/src/eval.c:3409:9
#9 0x66bcc3 in eval2 /home/jamessan/src/github.com/vim/src/eval.c:3341:9
#10 0x652332 in eval1 /home/jamessan/src/github.com/vim/src/eval.c:3269:9
#11 0x69703c in ex_echo /home/jamessan/src/github.com/vim/src/eval.c:8182:6
#12 0x84e6f1 in do_one_cmd /home/jamessan/src/github.com/vim/src/ex_docmd.c:2961:2
#13 0x82e01f in do_cmdline /home/jamessan/src/github.com/vim/src/ex_docmd.c:1110:17
#14 0xcd95b0 in nv_colon /home/jamessan/src/github.com/vim/src/normal.c:5398:15
#15 0xc7ed38 in normal_cmd /home/jamessan/src/github.com/vim/src/normal.c:1149:5
#16 0x14fa36b in main_loop /home/jamessan/src/github.com/vim/src/main.c:1311:6
#17 0x14f174e in vim_main2 /home/jamessan/src/github.com/vim/src/main.c:877:5
#18 0x14e3359 in main /home/jamessan/src/github.com/vim/src/main.c:415:12
#19 0x7f2ded9e42b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
#20 0x41d8a9 in _start (/home/jamessan/src/github.com/vim/src/vim+0x41d8a9)

SUMMARY: AddressSanitizer: undefined-behavior evalfunc.c:3341:29 in


:echo float2nr(pow(2, 64)) + float2nr(pow(2, 64))
eval.c:3934:12: runtime error: signed integer overflow: 9223372036854775807 + 9223372036854775807 cannot be represented in type 'long'
#0 0x6c10d1 in eval5 /home/jamessan/src/github.com/vim/src/eval.c:3934:12
#1 0x6badf1 in eval4 /home/jamessan/src/github.com/vim/src/eval.c:3492:9
#2 0x6ba413 in eval3 /home/jamessan/src/github.com/vim/src/eval.c:3409:9
#3 0x66bcc3 in eval2 /home/jamessan/src/github.com/vim/src/eval.c:3341:9
#4 0x652332 in eval1 /home/jamessan/src/github.com/vim/src/eval.c:3269:9
#5 0x69703c in ex_echo /home/jamessan/src/github.com/vim/src/eval.c:8182:6
#6 0x84e751 in do_one_cmd /home/jamessan/src/github.com/vim/src/ex_docmd.c:2961:2
#7 0x82e07f in do_cmdline /home/jamessan/src/github.com/vim/src/ex_docmd.c:1110:17
#8 0xcd9610 in nv_colon /home/jamessan/src/github.com/vim/src/normal.c:5398:15
#9 0xc7ed98 in normal_cmd /home/jamessan/src/github.com/vim/src/normal.c:1149:5
#10 0x14fa3cb in main_loop /home/jamessan/src/github.com/vim/src/main.c:1311:6
#11 0x14f17ae in vim_main2 /home/jamessan/src/github.com/vim/src/main.c:877:5
#12 0x14e33b9 in main /home/jamessan/src/github.com/vim/src/main.c:415:12
#13 0x7ff6465122b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
#14 0x41d8a9 in _start (/home/jamessan/src/github.com/vim/src/vim+0x41d8a9)

SUMMARY: AddressSanitizer: undefined-behavior eval.c:3934:12 in


:echo float2nr(-1 * pow(2, 64)) - float2nr(pow(2, 64))
eval.c:3936:12: runtime error: signed integer overflow: -9223372036854775808 - 9223372036854775807 cannot be represented in type 'long'
#0 0x6c112d in eval5 /home/jamessan/src/github.com/vim/src/eval.c:3936:12
#1 0x6badf1 in eval4 /home/jamessan/src/github.com/vim/src/eval.c:3492:9
#2 0x6ba413 in eval3 /home/jamessan/src/github.com/vim/src/eval.c:3409:9
#3 0x66bcc3 in eval2 /home/jamessan/src/github.com/vim/src/eval.c:3341:9
#4 0x652332 in eval1 /home/jamessan/src/github.com/vim/src/eval.c:3269:9
#5 0x69703c in ex_echo /home/jamessan/src/github.com/vim/src/eval.c:8182:6
#6 0x84e751 in do_one_cmd /home/jamessan/src/github.com/vim/src/ex_docmd.c:2961:2
#7 0x82e07f in do_cmdline /home/jamessan/src/github.com/vim/src/ex_docmd.c:1110:17
#8 0xcd9610 in nv_colon /home/jamessan/src/github.com/vim/src/normal.c:5398:15
#9 0xc7ed98 in normal_cmd /home/jamessan/src/github.com/vim/src/normal.c:1149:5
#10 0x14fa3cb in main_loop /home/jamessan/src/github.com/vim/src/main.c:1311:6
#11 0x14f17ae in vim_main2 /home/jamessan/src/github.com/vim/src/main.c:877:5
#12 0x14e33b9 in main /home/jamessan/src/github.com/vim/src/main.c:415:12
#13 0x7f5bd1b452b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
#14 0x41d8a9 in _start (/home/jamessan/src/github.com/vim/src/vim+0x41d8a9)

SUMMARY: AddressSanitizer: undefined-behavior eval.c:3936:12 in

Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB

[Mike Williams]

Revised patch attached - makes more use of the new value limit constants
to remove magic constants in the source.

> Only the following runtime errors are remaining:
>
> eval.c:4068:15: runtime error: division by zero

This seems to be the following:

/* We rely on the floating point library to handle divide
* by zero to result in "inf" and not a crash. */
f1 = f1 / f2;

Floating point division by zero is undefined behaviour. It will need
the idiom used elsewhere to handle the -1/0/1 / 0.0 cases to generate
-inf/nan/inf as appropriate.

TTFN

Mike
--
'Where do these stairs go?' 'They go up.'

[Mike Williams]

Hi

Yep, there are quite a few issues in eval.c with signed overflow and fp
to signed integer assignment that will cause UB. It will take a while
to work through them. First I'll need to get a compiler that can do
these checks. Soon.

TTFN

Mike
--
Ligneous and petrous projectiles can potentially fracture my osseous
structure, but pejorative appellations will forever remain innocuous.