* [PATCH] Fix aliasing violation in __vfscanf_internal [BZ #26690]
@ 2020-10-07 17:50 Szabolcs Nagy
2020-10-07 18:08 ` Florian Weimer
0 siblings, 1 reply; 5+ messages in thread
From: Szabolcs Nagy @ 2020-10-07 17:50 UTC (permalink / raw)
To: libc-alpha
Internal stdio code uses both CHAR_T and UCHAR_T strings.
But the internal helper read_int was only written for UCHAR_T.
A CHAR_T object can alias UCHAR_T, but CHAR_T * cannot alias
UCHAR_T *. This means a cast like (UCHAR_T **)&pc is likely
a bug in the code (currently GCC does not warn about this
see PR97321). The fix introduces a read_int_char variant and
removes the problematic casts.
(The mixed use of CHAR_T and UCHAR_T may be a design mistake
in stdio: if everything used char and wchar_t consistently
then aliasing violation would be much less likely, but fixing
that requires more refactoring.)
Fixes bug 26690.
---
stdio-common/vfscanf-internal.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
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;
--
2.17.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix aliasing violation in __vfscanf_internal [BZ #26690]
2020-10-07 17:50 [PATCH] Fix aliasing violation in __vfscanf_internal [BZ #26690] Szabolcs Nagy
@ 2020-10-07 18:08 ` Florian Weimer
2020-10-07 18:10 ` Adhemerval Zanella
0 siblings, 1 reply; 5+ messages in thread
From: Florian Weimer @ 2020-10-07 18:08 UTC (permalink / raw)
To: Szabolcs Nagy via Libc-alpha
* 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix aliasing violation in __vfscanf_internal [BZ #26690]
2020-10-07 18:08 ` Florian Weimer
@ 2020-10-07 18:10 ` Adhemerval Zanella
2020-10-07 18:16 ` Florian Weimer
0 siblings, 1 reply; 5+ messages in thread
From: Adhemerval Zanella @ 2020-10-07 18:10 UTC (permalink / raw)
To: libc-alpha, Florian Weimer, Szabolcs Nagy
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix aliasing violation in __vfscanf_internal [BZ #26690]
2020-10-07 18:10 ` Adhemerval Zanella
@ 2020-10-07 18:16 ` Florian Weimer
2020-10-08 7:27 ` Szabolcs Nagy
0 siblings, 1 reply; 5+ messages in thread
From: Florian Weimer @ 2020-10-07 18:16 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, Szabolcs Nagy, schwab
* 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix aliasing violation in __vfscanf_internal [BZ #26690]
2020-10-07 18:16 ` Florian Weimer
@ 2020-10-08 7:27 ` Szabolcs Nagy
0 siblings, 0 replies; 5+ messages in thread
From: Szabolcs Nagy @ 2020-10-08 7:27 UTC (permalink / raw)
To: Florian Weimer; +Cc: Adhemerval Zanella, libc-alpha, schwab
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-08 7:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 17:50 [PATCH] Fix aliasing violation in __vfscanf_internal [BZ #26690] Szabolcs Nagy
2020-10-07 18:08 ` Florian Weimer
2020-10-07 18:10 ` Adhemerval Zanella
2020-10-07 18:16 ` Florian Weimer
2020-10-08 7:27 ` Szabolcs Nagy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).