Message ID | 20201007175049.32564-1-szabolcs.nagy@arm.com |
---|---|
State | New |
Headers | show |
Series | Fix aliasing violation in __vfscanf_internal [BZ #26690] | expand |
* Szabolcs Nagy via Libc-alpha: > diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c > index 95b46dcbeb..d832493623 100644 > --- a/stdio-common/vfscanf-internal.c > +++ b/stdio-common/vfscanf-internal.c > @@ -135,6 +135,16 @@ > > #include "printf-parse.h" /* Use read_int. */ > > +/* Same as read_int, but for CHAR_T * instead of UCHAR_T * string. */ > +static int > +read_int_char (const CHAR_T * *pstr) > +{ > + const UCHAR_T *ustr = (const UCHAR_T *) *pstr; > + int retval = read_int (&ustr); > + *pstr = (const CHAR_T *) ustr; > + return retval; > +} > + > #define encode_error() do { \ > __set_errno (EILSEQ); \ > goto errout; \ > @@ -486,7 +496,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, > /* Check for a positional parameter specification. */ > if (ISDIGIT ((UCHAR_T) *f)) > { > - argpos = read_int ((const UCHAR_T **) &f); > + argpos = read_int_char (&f); > if (*f == L_('$')) > ++f; > else > @@ -522,7 +532,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, > /* Find the maximum field width. */ > width = 0; > if (ISDIGIT ((UCHAR_T) *f)) > - width = read_int ((const UCHAR_T **) &f); > + width = read_int_char (&f); > got_width: > if (width == 0) > width = -1; Patch and commit message look reasonable to me. Thanks.
On 07/10/2020 15:08, Florian Weimer wrote: > * Szabolcs Nagy via Libc-alpha: > >> diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c >> index 95b46dcbeb..d832493623 100644 >> --- a/stdio-common/vfscanf-internal.c >> +++ b/stdio-common/vfscanf-internal.c >> @@ -135,6 +135,16 @@ >> >> #include "printf-parse.h" /* Use read_int. */ >> >> +/* Same as read_int, but for CHAR_T * instead of UCHAR_T * string. */ >> +static int >> +read_int_char (const CHAR_T * *pstr) >> +{ >> + const UCHAR_T *ustr = (const UCHAR_T *) *pstr; >> + int retval = read_int (&ustr); >> + *pstr = (const CHAR_T *) ustr; >> + return retval; >> +} >> + >> #define encode_error() do { \ >> __set_errno (EILSEQ); \ >> goto errout; \ >> @@ -486,7 +496,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, >> /* Check for a positional parameter specification. */ >> if (ISDIGIT ((UCHAR_T) *f)) >> { >> - argpos = read_int ((const UCHAR_T **) &f); >> + argpos = read_int_char (&f); >> if (*f == L_('$')) >> ++f; >> else >> @@ -522,7 +532,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, >> /* Find the maximum field width. */ >> width = 0; >> if (ISDIGIT ((UCHAR_T) *f)) >> - width = read_int ((const UCHAR_T **) &f); >> + width = read_int_char (&f); >> got_width: >> if (width == 0) >> width = -1; > > Patch and commit message look reasonable to me. Thanks. > Andreas has sent a similar fix for the same issue [1]. Is it something wrong with his fix? It seems to cover more aliasing violation [1] https://sourceware.org/pipermail/libc-alpha/2020-October/118149.html
* Adhemerval Zanella: > On 07/10/2020 15:08, Florian Weimer wrote: >> * Szabolcs Nagy via Libc-alpha: >> >>> diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c >>> index 95b46dcbeb..d832493623 100644 >>> --- a/stdio-common/vfscanf-internal.c >>> +++ b/stdio-common/vfscanf-internal.c >>> @@ -135,6 +135,16 @@ >>> >>> #include "printf-parse.h" /* Use read_int. */ >>> >>> +/* Same as read_int, but for CHAR_T * instead of UCHAR_T * string. */ >>> +static int >>> +read_int_char (const CHAR_T * *pstr) >>> +{ >>> + const UCHAR_T *ustr = (const UCHAR_T *) *pstr; >>> + int retval = read_int (&ustr); >>> + *pstr = (const CHAR_T *) ustr; >>> + return retval; >>> +} >>> + >>> #define encode_error() do { \ >>> __set_errno (EILSEQ); \ >>> goto errout; \ >>> @@ -486,7 +496,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, >>> /* Check for a positional parameter specification. */ >>> if (ISDIGIT ((UCHAR_T) *f)) >>> { >>> - argpos = read_int ((const UCHAR_T **) &f); >>> + argpos = read_int_char (&f); >>> if (*f == L_('$')) >>> ++f; >>> else >>> @@ -522,7 +532,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, >>> /* Find the maximum field width. */ >>> width = 0; >>> if (ISDIGIT ((UCHAR_T) *f)) >>> - width = read_int ((const UCHAR_T **) &f); >>> + width = read_int_char (&f); >>> got_width: >>> if (width == 0) >>> width = -1; >> >> Patch and commit message look reasonable to me. Thanks. >> > > Andreas has sent a similar fix for the same issue [1]. Is it something > wrong with his fix? It seems to cover more aliasing violation > > [1] https://sourceware.org/pipermail/libc-alpha/2020-October/118149.html The other patch looks reasonable to me as well, slightly better even. I had missed it, sorry. The additional changes are just shuffling harmless casts around, not fixes for more aliasing violations, as far as I can see.
The 10/07/2020 20:16, Florian Weimer wrote: > * Adhemerval Zanella: > > > On 07/10/2020 15:08, Florian Weimer wrote: > >> * Szabolcs Nagy via Libc-alpha: > >> > >>> diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c > >>> index 95b46dcbeb..d832493623 100644 > >>> --- a/stdio-common/vfscanf-internal.c > >>> +++ b/stdio-common/vfscanf-internal.c > >>> @@ -135,6 +135,16 @@ > >>> > >>> #include "printf-parse.h" /* Use read_int. */ > >>> > >>> +/* Same as read_int, but for CHAR_T * instead of UCHAR_T * string. */ > >>> +static int > >>> +read_int_char (const CHAR_T * *pstr) > >>> +{ > >>> + const UCHAR_T *ustr = (const UCHAR_T *) *pstr; > >>> + int retval = read_int (&ustr); > >>> + *pstr = (const CHAR_T *) ustr; > >>> + return retval; > >>> +} > >>> + > >>> #define encode_error() do { \ > >>> __set_errno (EILSEQ); \ > >>> goto errout; \ > >>> @@ -486,7 +496,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, > >>> /* Check for a positional parameter specification. */ > >>> if (ISDIGIT ((UCHAR_T) *f)) > >>> { > >>> - argpos = read_int ((const UCHAR_T **) &f); > >>> + argpos = read_int_char (&f); > >>> if (*f == L_('$')) > >>> ++f; > >>> else > >>> @@ -522,7 +532,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, > >>> /* Find the maximum field width. */ > >>> width = 0; > >>> if (ISDIGIT ((UCHAR_T) *f)) > >>> - width = read_int ((const UCHAR_T **) &f); > >>> + width = read_int_char (&f); > >>> got_width: > >>> if (width == 0) > >>> width = -1; > >> > >> Patch and commit message look reasonable to me. Thanks. > >> > > > > Andreas has sent a similar fix for the same issue [1]. Is it something > > wrong with his fix? It seems to cover more aliasing violation > > > > [1] https://sourceware.org/pipermail/libc-alpha/2020-October/118149.html > > The other patch looks reasonable to me as well, slightly better even. > I had missed it, sorry. me too. > > The additional changes are just shuffling harmless casts around, not > fixes for more aliasing violations, as far as I can see.
diff --git a/stdio-common/vfscanf-internal.c b/stdio-common/vfscanf-internal.c index 95b46dcbeb..d832493623 100644 --- a/stdio-common/vfscanf-internal.c +++ b/stdio-common/vfscanf-internal.c @@ -135,6 +135,16 @@ #include "printf-parse.h" /* Use read_int. */ +/* Same as read_int, but for CHAR_T * instead of UCHAR_T * string. */ +static int +read_int_char (const CHAR_T * *pstr) +{ + const UCHAR_T *ustr = (const UCHAR_T *) *pstr; + int retval = read_int (&ustr); + *pstr = (const CHAR_T *) ustr; + return retval; +} + #define encode_error() do { \ __set_errno (EILSEQ); \ goto errout; \ @@ -486,7 +496,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, /* Check for a positional parameter specification. */ if (ISDIGIT ((UCHAR_T) *f)) { - argpos = read_int ((const UCHAR_T **) &f); + argpos = read_int_char (&f); if (*f == L_('$')) ++f; else @@ -522,7 +532,7 @@ __vfscanf_internal (FILE *s, const char *format, va_list argptr, /* Find the maximum field width. */ width = 0; if (ISDIGIT ((UCHAR_T) *f)) - width = read_int ((const UCHAR_T **) &f); + width = read_int_char (&f); got_width: if (width == 0) width = -1;