Fix crash in linearize_compound_statement()
RFC PATCH 1/1 Fix crash in linearize_compound_statement()
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:
linearize.c +1 -1
@@ -1633,7 +1633,7 @@ static pseudo_t linearize_compound_statement(struct entrypoint *ep, struct statestruct 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 ) {
Signature · 2 lines
Regards, Pavel Roskin
Christopher Li
maintainerQuoted 9 lines · Pavel Roskin
static int x;
static inline void foo(void){ if (x) x = 1; } static void bar(void) { foo();
Quoted 8 lines
}
static typeof(bar) quux;The crash happens in
linearize_compound_statement(), and I believe that the reason is incorrect access tophi_node->phi_listwithout making sure thatphi_node->opcodeisOP_PHI. When processing the above program,phi_node->phi_listcan beOP_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.
Quoted 5 lines
I understand very little in sparse code, and I have no idea what kind of fallback is needed when
phi_node->opcodeis notOP_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
Quoted 14 lines · Christopher Li
Quoted 13 lines · Pavel Roskin
static int x;
static inline void foo(void){ if (x) x = 1; } static void bar(void) { foo();}
static typeof(bar) quux;
Quoted 8 lines
Quoted 4 lines
The crash happens in
linearize_compound_statement(), and I believe that the reason is incorrect access tophi_node->phi_listwithout making sure thatphi_node->opcodeisOP_PHI. When processing the above program,phi_node->phi_listcan beOP_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.
Quoted 6 lines
Quoted 4 lines
I understand very little in sparse code, and I have no idea what kind of fallback is needed when
phi_node->opcodeis notOP_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.
Quoted 1 line
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.
Quoted 3 lines
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.
Signature · 2 lines
Regards, Pavel Roskin
Christopher Li
maintainerQuoted 8 lines · Pavel Roskin
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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 2 lines
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