On 05/19/2017 08:27 AM, Bruno Haible wrote:
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.
>From 3f11d42a0e5afc742a0269224ea0236a6b65a47c Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Fri, 19 May 2017 15:35:24 -0700
Subject: [PATCH 1/2] argp: fix pointer-subtraction bug
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/argp-help.c (hol_append): Donât subtract pointers to
different arrays, as this can run afoul of -fcheck-pointer-bounds.
See the thread containing Bruno Haibleâs report in:
http://lists.gnu.org/archive/html/bug-gnulib/2017-05/msg00171.html
---
ChangeLog | 8 ++++++++
lib/argp-help.c | 3 ++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index 889ef11..b214252 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2017-05-19 Paul Eggert <egg...@cs.ucla.edu>
+
+ argp: fix pointer-subtraction bug
+ * lib/argp-help.c (hol_append): Donât subtract pointers to
+ different arrays, as this can run afoul of -fcheck-pointer-bounds.
+ See the thread containing Bruno Haibleâs report in:
+ http://lists.gnu.org/archive/html/bug-gnulib/2017-05/msg00171.html
+
2017-05-19 Bruno Haible <br...@clisp.org>
printf-posix tests: Avoid test failure with "gcc --coverage".
diff --git a/lib/argp-help.c b/lib/argp-help.c
index 0632960..ce9bab9 100644
--- a/lib/argp-help.c
+++ b/lib/argp-help.c
@@ -880,7 +880,8 @@ hol_append (struct hol *hol, struct hol *more)
/* Fix up the short options pointers from HOL. */
for (e = entries, left = hol->num_entries; left > 0; e++, left--)
- e->short_options += (short_options - hol->short_options);
+ e->short_options
+ = short_options + (e->short_options - hol->short_options);
/* Now add the short options from MORE, fixing up its entries
too. */
--
2.9.4
>From b592d9041eb083e23cee462dbc27b8cc25b52917 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Fri, 19 May 2017 15:39:06 -0700
Subject: [PATCH 2/2] argp: fix shift bug
* lib/argp-parse.c (parser_parse_opt): Rework to avoid undefined
behavior on shift overflow, caught by gcc -fsanitize=undefined.
---
ChangeLog | 4 ++++
lib/argp-parse.c | 22 ++++++++++++++++------
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index b214252..46714cd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
2017-05-19 Paul Eggert <egg...@cs.ucla.edu>
+ argp: fix shift bug
+ * lib/argp-parse.c (parser_parse_opt): Rework to avoid undefined
+ behavior on shift overflow, caught by gcc -fsanitize=undefined.
+
argp: fix pointer-subtraction bug
* lib/argp-help.c (hol_append): Donât subtract pointers to
different arrays, as this can run afoul of -fcheck-pointer-bounds.
diff --git a/lib/argp-parse.c b/lib/argp-parse.c
index 3f723bc..4723a2b 100644
--- a/lib/argp-parse.c
+++ b/lib/argp-parse.c
@@ -740,12 +740,22 @@ parser_parse_opt (struct parser *parser, int opt, char *val)
}
}
else
- /* A long option. We use shifts instead of masking for extracting
- the user value in order to preserve the sign. */
- err =
- group_parse (&parser->groups[group_key - 1], &parser->state,
- (opt << GROUP_BITS) >> GROUP_BITS,
- parser->opt_data.optarg);
+ /* 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));
+ err =
+ group_parse (&parser->groups[group_key - 1], &parser->state,
+ user_key, parser->opt_data.optarg);
+ }
if (err == EBADKEY)
/* At least currently, an option not recognized is an error in the
--
2.9.4
