Discussion:
results of gnulib tests with -fcheck-pointer-bounds
(too old to reply)
Bruno Haible
2017-05-19 15:27:28 UTC
Permalink
Raw Message
Here are the results of running a gnulib testdir with
CFLAGS="-O -ggdb -fcheck-pointer-bounds -mmpx"
on Linux/x86_64.

The interesting finding is in test-argp-2.sh. The message
"Saw a #BR!" is a bit cryptic, but is explained in
https://github.com/google/sanitizers/wiki/AddressSanitizerIntelMemoryProtectionExtensions

It gives us a list of addresses where bounds checks failed.
Namely:
$ gdb test-argp
(gdb) break *0x408d54
Breakpoint 1 at 0x408d54: file ../../gllib/argp-help.c, line 560.
(gdb) break *0x408d4e
Breakpoint 2 at 0x408d4e: file ../../gllib/argp-help.c, line 560.
(gdb) break *0x40c327
Breakpoint 3 at 0x40c327: file ../../gllib/argp-help.c, line 1112.
(gdb) break *0x40c4c2
Breakpoint 4 at 0x40c4c2: file ../../gllib/argp-help.c, line 1119.
(gdb) break *0x40c4bd
Breakpoint 5 at 0x40c4bd: file ../../gllib/argp-help.c, line 1119.

Does someone understand this argp-help.c code?

Bruno



FAIL: test-argp-2.sh
====================

*** argp.24014 Fri May 19 03:16:51 2017
--- - Fri May 19 03:16:51 2017
***************
*** 1,3 ****
--- 1,28 ----
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
Usage: test-argp [-tvCSOlp?V] [-f FILE] [-r FILE] [-o[ARG]] [--test]
[--file=FILE] [--input=FILE] [--read=FILE] [--verbose] [--cantiga]
[--sonet] [--option] [--optional[=ARG]] [--many] [--one] [--two]
*** argp.24014 Fri May 19 03:16:51 2017
--- - Fri May 19 03:16:51 2017
***************
*** 1,3 ****
--- 1,28 ----
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
Usage: test-argp [-tvCSOlp?V] [-f FILE] [-r FILE] [-o[ARG]] [--test]
[--file=FILE] [--input=FILE] [--read=FILE] [--verbose] [--cantiga] [--sonet]
[--option] [--optional[=ARG]] [--many] [--one] [--two] [--limerick] [--poem]
*** argp.24014 Fri May 19 03:16:51 2017
--- - Fri May 19 03:16:51 2017
***************
*** 1,3 ****
--- 1,28 ----
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d4e
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x408d54
+ Saw a #BR! status 1 at 0x40c327
+ Saw a #BR! status 1 at 0x40c4c2
+ Saw a #BR! status 1 at 0x40c322
+ Saw a #BR! status 1 at 0x40c4bd
+ Saw a #BR! status 1 at 0x40c322
+ Saw a #BR! status 1 at 0x40c4bd
+ Saw a #BR! status 1 at 0x40c322
+ Saw a #BR! status 1 at 0x40c4bd
+ Saw a #BR! status 1 at 0x40c322
+ Saw a #BR! status 1 at 0x40c4bd
Usage: test-argp [OPTION...] ARGS...
documentation string

FAIL test-argp-2.sh (exit status: 1)

FAIL: test-fprintf-posix2.sh
============================

Unexpected trap 0! at 0x400946
Aborted (core dumped)
FAIL test-fprintf-posix2.sh (exit status: 1)

FAIL: test-printf-posix2.sh
===========================

Unexpected trap 0! at 0x400906
Aborted (core dumped)
FAIL test-printf-posix2.sh (exit status: 1)
Paul Eggert
2017-05-19 22:48:51 UTC
Permalink
Raw Message
The message "Saw a #BR!" is a bit cryptic
An understatement to be sure. In my experience, even when you know
exactly which machine instruction is trapping and know which source-code
statement it corresponds to, it's often tricky to puzzle out why an
-fcheck-pointer-bounds failure occurred. So far I haven't been bold
enough to give a tricky problem like that to my undergraduate students.
Maybe in a year or two the debugging tools will be better. (Plus, I have
to wait for our university to get teaching servers new enough to support
MPX.)
Does someone understand this argp-help.c code?
I didn't, but after looking at the code for a bit I see a problem that
could explain the symptoms you observe. hol_append subtracts pointers
into different arrays, which has undefined behavior in C, and
-fcheck-pointer-bounds can catch this after the resulting offset is used
to calculate a pointer and the pointer is then later used. This is
clearly a portability bug so I installed the first attached patch. Does
it fix the problem on your platform?

I also tested argp under -fsanitize=undefined and found a different bug,
fixed in the 2nd attached patch.
Bruno Haible
2017-05-20 13:06:39 UTC
Permalink
Raw Message
Hi Paul,
Post by Paul Eggert
The message "Saw a #BR!" is a bit cryptic
Does someone understand this argp-help.c code?
I didn't, but after looking at the code for a bit I see a problem that
could explain the symptoms you observe. hol_append subtracts pointers
into different arrays, which has undefined behavior in C, and
-fcheck-pointer-bounds can catch this after the resulting offset is used
to calculate a pointer and the pointer is then later used. This is
clearly a portability bug so I installed the first attached patch. Does
it fix the problem on your platform?
Yes, it fixes the problem. Great! Thanks.
Post by Paul Eggert
I also tested argp under -fsanitize=undefined and found a different bug,
fixed in the 2nd attached patch.
Sorry, I don't understand this patch: it avoids the undefined behaviour
on left-shift, but still makes assumptions about the "implementation-
defined" behaviour of right-shift [1], here:
user_key = shifted_user_key >> GROUP_BITS;
[1] http://stackoverflow.com/questions/7622/are-the-shift-operators-arithmetic-or-logical-in-c

How about this? The intended operation (in terms of bits) is:

z.......abcdefghijklmnopqrstuvwx
-> zzzzzzzzabcdefghijklmnopqrstuvwx

AFAIK, bit operations (&, |, ~) don't have undefined or implementation-defined
behaviour on signed integer types. Once we assume two's complement - which your
comment already states - they are unambiguous. Proposed patch:


2017-05-20 Bruno Haible <***@clisp.org>

argp: Simplify bit manipulation.
* lib/argp-parse.c (parser_parse_opt): Use &, |, ~ instead of shifts
on a signed integer type.

diff --git a/lib/argp-parse.c b/lib/argp-parse.c
index 4723a2b..fb04a4a 100644
--- a/lib/argp-parse.c
+++ b/lib/argp-parse.c
@@ -743,15 +743,8 @@ parser_parse_opt (struct parser *parser, int opt, char *val)
/* A long option. Preserve the sign in the user key, without
invoking undefined behavior. Assume two's complement. */
{
- unsigned uopt = opt;
- unsigned ushifted_user_key = uopt << GROUP_BITS;
- int shifted_user_key = ushifted_user_key;
- int user_key;
- if (-1 >> 1 == -1)
- user_key = shifted_user_key >> GROUP_BITS;
- else
- user_key = ((ushifted_user_key >> GROUP_BITS)
- - (shifted_user_key < 0 ? 1 << USER_BITS : 0));
+ int user_key =
+ ((opt & (1 << (USER_BITS - 1))) ? ~USER_MASK : 0) | (opt & USER_MASK);
err =
group_parse (&parser->groups[group_key - 1], &parser->state,
user_key, parser->opt_data.optarg);
Bruno Haible
2017-05-20 17:30:17 UTC
Permalink
Raw Message
Post by Bruno Haible
z.......abcdefghijklmnopqrstuvwx
-> zzzzzzzzabcdefghijklmnopqrstuvwx
Correction: It is like this:

........abcdefghijklmnopqrstuvwx
-> aaaaaaaaabcdefghijklmnopqrstuvwx

Bruno
Paul Eggert
2017-05-20 20:07:18 UTC
Permalink
Raw Message
Post by Bruno Haible
+ int user_key =
+ ((opt & (1 << (USER_BITS - 1))) ? ~USER_MASK : 0) | (opt & USER_MASK);
Yes, thanks, that's simpler than what is there now, and (in theory at least) is
more portable.
Bruno Haible
2017-05-21 00:47:47 UTC
Permalink
Raw Message
Post by Paul Eggert
Post by Bruno Haible
+ int user_key =
+ ((opt & (1 << (USER_BITS - 1))) ? ~USER_MASK : 0) | (opt & USER_MASK);
Yes, thanks, that's simpler than what is there now, and (in theory at least) is
more portable.
OK, pushed.

Loading...