Re: [hackers] [scc] [cc1] Fix valid_va_list || Quentin Rameau

From: Michael Forney <mforney_AT_mforney.org>
Date: Wed, 15 Feb 2017 09:36:25 -0800

On Wed, Feb 15, 2017 at 3:57 AM, <git_AT_suckless.org> wrote:
> commit 9f519ebee29edca444e31fb8f01348f35252a754
> Author: Quentin Rameau <quinq_AT_fifth.space>
> AuthorDate: Wed Feb 15 11:55:24 2017 +0100
> Commit: Quentin Rameau <quinq_AT_fifth.space>
> CommitDate: Wed Feb 15 12:58:41 2017 +0100
>
> [cc1] Fix valid_va_list
>
> Directly comparing objects via pointers is too strict and the check
> would fail for typedefs.

I'm only just now getting up to speed on the code base, but I think
this commit is wrong. There is no "op" for typedef. When a typedef
declaration is processed, a new TYPEIDEN symbol is added with
sym->type set to the original type, and when a typedef is used in a
declaration, it uses the TYPEIDEN symbol's type (just as if the
original type was used directly).

I believe the underlying issue that you are running into is that
eqtype for struct types is broken. If two struct types have the same
number of elements, eqtype tries to compare the p.pars (which are Type
*) of the two types and structs use the p.fields union field (which
are Symbol *).

For va_type, it is defined as a struct with n.elem == 0. This causes
it to compare equally to all forward declarations of structs (which
also have n.elem == 0). In my case trying to compile libutil/eprintf.c
from sbase, PTR to va_type hashed and compared equally to _IO_FILE,
which caused the check to fail. I first thought to fix this by adding
a check tp1->letter == tp2->letter in eqtype, but I think the problem
is more general. I also hit some crashes for two struct types that had
the same number of elements (struct timeval and regmatch_t) but hashed
the same, since it tried to compare the field symbols as Type *.

I'm not sure of how to fix eqtype for STRUCT or UNION types, but I
think that this is the root cause of the problem you ran into. I think
this workaround just masks the real issue.

(I spent a while debugging what I think is the same bug a couple days ago)

> diff --git a/cc1/arch/amd64-sysv/arch.c b/cc1/arch/amd64-sysv/arch.c
> index 89bec2d..2899034 100644
> --- a/cc1/arch/amd64-sysv/arch.c
> +++ b/cc1/arch/amd64-sysv/arch.c
> _AT_@ -218,5 +218,5 @@ iarch(void)
> int
> valid_va_list(Type *tp)
> {
> - return tp->op == PTR && tp->type == va_type;
> + return tp->op == PTR && eqtype(tp->type, va_type, 1);
> }
> diff --git a/cc1/arch/i386-sysv/arch.c b/cc1/arch/i386-sysv/arch.c
> index 6420002..64ab6b2 100644
> --- a/cc1/arch/i386-sysv/arch.c
> +++ b/cc1/arch/i386-sysv/arch.c
> _AT_@ -219,5 +219,5 @@ iarch(void)
> int
> valid_va_list(Type *tp)
> {
> - return tp == va_list_type;
> + return eqtype(tp, va_list_type, 1);
> }
> diff --git a/cc1/arch/qbe/arch.c b/cc1/arch/qbe/arch.c
> index 3835ed1..18852bf 100644
> --- a/cc1/arch/qbe/arch.c
> +++ b/cc1/arch/qbe/arch.c
> _AT_@ -218,5 +218,5 @@ iarch(void)
> int
> valid_va_list(Type *tp)
> {
> - return tp->op == PTR && tp->type == va_type;
> + return tp->op == PTR && eqtype(tp->type, va_type, 1);
> }
> diff --git a/cc1/arch/z80/arch.c b/cc1/arch/z80/arch.c
> index 70aec61..f7b50b5 100644
> --- a/cc1/arch/z80/arch.c
> +++ b/cc1/arch/z80/arch.c
> _AT_@ -217,5 +217,5 @@ iarch(void)
> int
> valid_va_list(Type *tp)
> {
> - return tp == va_list_type;
> + return eqtype(tp, va_list_type, 1);
> }
>
Received on Wed Feb 15 2017 - 18:36:25 CET