| Message ID | 20250507142110.3452012-11-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add initial support for --enable-ubsan | expand |
* Adhemerval Zanella: >>From gnulib commits 06094e390b0 and 88033d3779362a. > --- > argp/argp-parse.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/argp/argp-parse.c b/argp/argp-parse.c > index 82c7b784de..99f8d9ecd4 100644 > --- a/argp/argp-parse.c > +++ b/argp/argp-parse.c > @@ -735,12 +735,15 @@ 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. */ > + { > + int user_key = > + ((opt & (1 << (USER_BITS - 1))) ? ~USER_MASK : 0) | (opt & USER_MASK); Would this be clearer? (int) ((unsigned int) opt << GROUP_BITS) >> GROUP_BITS, Or does ubsan flag that as well? Conversion to negative int is a GCC extension: | For conversion to a type of width N, the value is reduced modulo 2^N | to be within range of the type; no signal is raised. <https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Integers-implementation.html>
On 07/05/25 16:42, Florian Weimer wrote: > * Adhemerval Zanella: > >> >From gnulib commits 06094e390b0 and 88033d3779362a. >> --- >> argp/argp-parse.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/argp/argp-parse.c b/argp/argp-parse.c >> index 82c7b784de..99f8d9ecd4 100644 >> --- a/argp/argp-parse.c >> +++ b/argp/argp-parse.c >> @@ -735,12 +735,15 @@ 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. */ >> + { >> + int user_key = >> + ((opt & (1 << (USER_BITS - 1))) ? ~USER_MASK : 0) | (opt & USER_MASK); > > Would this be clearer? > > (int) ((unsigned int) opt << GROUP_BITS) >> GROUP_BITS, This would be another gnulib deviation, and the code comes from gnulib. I think we should keep it simple to make the sync more straightforward. > > Or does ubsan flag that as well? Conversion to negative int is a GCC > extension: > > | For conversion to a type of width N, the value is reduced modulo 2^N > | to be within range of the type; no signal is raised. Yeah, but -fsanitize=undefinied still triggers this as UB. I can add an option to suppress this kind of shift, but this will add a bit more complexity on the handler handling. > > <https://gcc.gnu.org/onlinedocs/gcc-15.1.0/gcc/Integers-implementation.html>
diff --git a/argp/argp-parse.c b/argp/argp-parse.c index 82c7b784de..99f8d9ecd4 100644 --- a/argp/argp-parse.c +++ b/argp/argp-parse.c @@ -735,12 +735,15 @@ 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. */ + { + 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); + } if (err == EBADKEY) /* At least currently, an option not recognized is an error in the