[RFC PATCH] Fix crash in linearize_compound_statement()

[RFC PATCH] Fix crash in linearize_compound_statement()

[Pavel Roskin]

Hello!

The current sparse crashes on this program:


static int x;
static inline void foo(void)
{
    if (x)
	x = 1;
}
static void bar(void)
{
    foo();
}
static typeof(bar) quux;


The crash happens in linearize_compound_statement(), and I believe that
the reason is incorrect access to phi_node->phi_list without making sure
that phi_node->opcode is OP_PHI.  When processing the above program,
phi_node->phi_list can be OP_INLINED_CALL.

I understand very little in sparse code, and I have no idea what kind of
fallback is needed when phi_node->opcode is not OP_PHI.

But this patch fixes the crash:


diff --git a/linearize.c b/linearize.c
index 8a68f05..ff4f3b6 100644
--- a/linearize.c
+++ b/linearize.c
@@ -1633,7 +1633,7 @@ static pseudo_t linearize_compound_statement(struct entrypoint *ep, struct state
 		struct basic_block *bb = add_label(ep, ret);
 		struct instruction *phi_node = first_instruction(bb->insns);
 
-		if (!phi_node)
+		if (!phi_node || phi_node->opcode != OP_PHI)
 			return pseudo;
 
 		if (pseudo_list_size(phi_node->phi_list)==1) {


-- 
Regards,
Pavel Roskin

Re: [RFC PATCH] Fix crash in linearize_compound_statement()

[Christopher Li]

On Mon, Apr 7, 2008 at 1:45 PM, Pavel Roskin <proski@gnu.org> wrote:
>  static int x;
>  static inline void foo(void)
>  {
>     if (x)
>         x = 1;
>  }
>  static void bar(void)
>  {
>     foo();
>  }
>  static typeof(bar) quux;
>
>
>  The crash happens in linearize_compound_statement(), and I believe that
>  the reason is incorrect access to phi_node->phi_list without making sure
>  that phi_node->opcode is OP_PHI.  When processing the above program,
>  phi_node->phi_list can be OP_INLINED_CALL.

No, that is not the root cause. The root cause is you feed sparse with
bad input. typeof(bar) will give you a type of a function. Sparse just let
quux have the base type of bar function body. That is just so wrong.

>
>  I understand very little in sparse code, and I have no idea what kind of
>  fallback is needed when phi_node->opcode is not OP_PHI.
>
>  But this patch fixes the crash:

As I said before, this is not the right fix. Wish it not get applied.

I agree sparse should not assert on it, but not like this.
You should at least fix it from the typeof(bar) side, instead of try
to linearize
the same function twice.

Chris

Re: [RFC PATCH] Fix crash in linearize_compound_statement()

[Pavel Roskin]

On Mon, 2008-04-07 at 14:55 -0700, Christopher Li wrote:
> On Mon, Apr 7, 2008 at 1:45 PM, Pavel Roskin <proski@gnu.org> wrote:
> >  static int x;
> >  static inline void foo(void)
> >  {
> >     if (x)
> >         x = 1;
> >  }
> >  static void bar(void)
> >  {
> >     foo();
> >  }
> >  static typeof(bar) quux;
> >
> >
> >  The crash happens in linearize_compound_statement(), and I believe that
> >  the reason is incorrect access to phi_node->phi_list without making sure
> >  that phi_node->opcode is OP_PHI.  When processing the above program,
> >  phi_node->phi_list can be OP_INLINED_CALL.
> 
> No, that is not the root cause. The root cause is you feed sparse with
> bad input. typeof(bar) will give you a type of a function. Sparse just let
> quux have the base type of bar function body. That is just so wrong.

I know.  And the source that was doing it is fixed long ago.

> >  I understand very little in sparse code, and I have no idea what kind of
> >  fallback is needed when phi_node->opcode is not OP_PHI.
> >
> >  But this patch fixes the crash:
> 
> As I said before, this is not the right fix. Wish it not get applied.

I should have been more clear.  Last time I was carried away by some
strange messages about "return" and labels, which seemed really weird to
me, but less so after your explanation.

This example is made from the same original source, but this time I
concentrated on the crash, because that's the thing that cannot be
justified by any bad code.

> I agree sparse should not assert on it, but not like this.

I don't quite understand why sparse is getting in this situation, but
I'm quite confident from seeing other code that phi_node->phi_list is
invalid if phi_node->opcode is not OP_PHI, and should not be accessed.
I'd rather see sparse tripping an assert and crashing before it happens
rather than see it interpreting some other data as phi_list and crashing
randomly while iterating over that bogus list.

> You should at least fix it from the typeof(bar) side, instead of try
> to linearize
> the same function twice.

That's fixed, of course.  And many other problems are fixed too, some of
which are user visible.  I really appreciate the usefulness of sparse.

All I'm trying to do now is to have that crash fixed, after which I
would be glad to forget the ugly code that caused it.

-- 
Regards,
Pavel Roskin

Re: [RFC PATCH] Fix crash in linearize_compound_statement()

[Christopher Li]

On Mon, Apr 7, 2008 at 10:14 PM, Pavel Roskin <proski@gnu.org> wrote:
>
>  I should have been more clear.  Last time I was carried away by some
>  strange messages about "return" and labels, which seemed really weird to
>  me, but less so after your explanation.
>
>  This example is made from the same original source, but this time I
>  concentrated on the crash, because that's the thing that cannot be
>  justified by any bad code.

I understand that you have your source code which sparse choked on
fixed. Good.

I am actually complaining your fix to sparse itself. It is not good enough.
You fix it in the linearize stage. That is already too late.

You should fix it in the parse and type evaluate stage, before it even reach
to the linearizer.

>
>
>  > I agree sparse should not assert on it, but not like this.
>
>  I don't quite understand why sparse is getting in this situation, but

Exactly. That is because you try to linearize the exact same function
twice due to the "typeof(bar)". Linearize the same function twice is just
plain wrong. It should never happen. Your patch just covers up the real problem
instead of fixing it.

>  That's fixed, of course.  And many other problems are fixed too, some of
>  which are user visible.  I really appreciate the usefulness of sparse.

I mean the fix in the sparse source code, not the source code which sparse
is checking. You should change sparse, when it do "typeof(function) varname",
give it a warning and skip this "varname" node all together. Later in the
linearize stage, it will not linearize the same function twice.

What your patch does is actually allowing sparse linearized a function twice.
That is going to mess up the user-define chain very badly (e.g. the PHI node
usage problem you are seeing.)

Chris