| Message ID | 20170820221602.27852-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
Hi Luc, On 20 August 2017 at 23:16, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > During the expansion of a dereference, it's if the initializer > which corrrespond to the offset we're interested is a constant. > In which case this dereference can be avoided and the value > given in the initializer can be used instead. > > However, it's not enough to check for the offset since for bitfields > several are placed at the same offset. > Currently, the first initializer matching the offset is selected. > > Fix this by refusing such expansion if the constant value correspond > to a bitfield. > Thanks very much for the fix! Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 20, 2017 at 6:16 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > During the expansion of a dereference, it's if the initializer > which corrrespond to the offset we're interested is a constant. > In which case this dereference can be avoided and the value > given in the initializer can be used instead. > > However, it's not enough to check for the offset since for bitfields > several are placed at the same offset. > Currently, the first initializer matching the offset is selected. > > Fix this by refusing such expansion if the constant value correspond > to a bitfield. > With this applied, sparse still have a bug at did not match the value to the bit field member, right? I saw on the other email thread said the value always pick offset zero. Should I apply this as patch format or wait for it show up in a git pull request later? > --- a/expand.c > +++ b/expand.c > @@ -644,6 +644,8 @@ static int expand_dereference(struct expression *expr) > if (value) { > /* FIXME! We should check that the size is right! */ > if (value->type == EXPR_VALUE) { > + if (is_bitfield_type(value->ctype)) > + return UNSAFE; You might want to consider move this outside of the EXPR_VALUE. I assume there is a bug in sparse matching the value to the member wrong, it could happen to EXPR_FVALUE as well. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chris, On 21 August 2017 at 13:26, Christopher Li <sparse@chrisli.org> wrote: > On Sun, Aug 20, 2017 at 6:16 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: >> During the expansion of a dereference, it's if the initializer >> which corrrespond to the offset we're interested is a constant. >> In which case this dereference can be avoided and the value >> given in the initializer can be used instead. >> >> However, it's not enough to check for the offset since for bitfields >> several are placed at the same offset. >> Currently, the first initializer matching the offset is selected. >> >> Fix this by refusing such expansion if the constant value correspond >> to a bitfield. >> > >> --- a/expand.c >> +++ b/expand.c >> @@ -644,6 +644,8 @@ static int expand_dereference(struct expression *expr) >> if (value) { >> /* FIXME! We should check that the size is right! */ >> if (value->type == EXPR_VALUE) { >> + if (is_bitfield_type(value->ctype)) >> + return UNSAFE; > > You might want to consider move this outside of the EXPR_VALUE. > I assume there is a bug in sparse matching the value to the member > wrong, it could happen to EXPR_FVALUE as well. > Since a bitfield is always integer and this issue is occurring with bitfields, then I guess this change is correct? Regards Dibyendu -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 21, 2017 at 9:32 AM, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote: >> You might want to consider move this outside of the EXPR_VALUE. >> I assume there is a bug in sparse matching the value to the member >> wrong, it could happen to EXPR_FVALUE as well. >> > > Since a bitfield is always integer and this issue is occurring with > bitfields, then I guess this change is correct? Ok, I forget about that. You are absolutely. right. Never mind that comment then. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 21, 2017 at 2:26 PM, Christopher Li <sparse@chrisli.org> wrote: > On Sun, Aug 20, 2017 at 6:16 PM, Luc Van Oostenryck wrote: > > With this applied, sparse still have a bug at did not match the > value to the bit field member, right? I saw on the other email > thread said the value always pick offset zero. I'm not sure to understand. Have you an example? > Should I apply this as patch format or wait for it show up in a > git pull request later? Wait for the pull request, please. >> --- a/expand.c >> +++ b/expand.c >> @@ -644,6 +644,8 @@ static int expand_dereference(struct expression *expr) >> if (value) { >> /* FIXME! We should check that the size is right! */ >> if (value->type == EXPR_VALUE) { >> + if (is_bitfield_type(value->ctype)) >> + return UNSAFE; > > You might want to consider move this outside of the EXPR_VALUE. > I assume there is a bug in sparse matching the value to the member > wrong, it could happen to EXPR_FVALUE as well. Well ..., if we have an EXPR_FVALUE which type is a bitfield, yes for sure there would be a bug. But I have no reason to belive there is such bug and since checking the ctype is more costly than checking the expr->type, I think it's best so. Note, I find this check already annoying and hackish if not worse. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 21, 2017 at 9:42 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Mon, Aug 21, 2017 at 2:26 PM, Christopher Li <sparse@chrisli.org> wrote: >> On Sun, Aug 20, 2017 at 6:16 PM, Luc Van Oostenryck wrote: >> >> With this applied, sparse still have a bug at did not match the >> value to the bit field member, right? I saw on the other email >> thread said the value always pick offset zero. > > I'm not sure to understand. > Have you an example? I am referenitng to your email reply to Dibyendu: =========================== It's a very surprising bug. It's not a linearization or an optimization bug as the AST is already wrong. With a simpler test case, like: struct s { char a:4; char b:4; }; int foo(void) { struct s x = { .a = 2, .b = 4 }; return x.b; } you can see that the linearization produce correct code for the initializer. You can also see that the return statement to be linearized is something like STMT_RETURN ret_value: EXPR_VALUE (value = 2) =========================== > > Wait for the pull request, please. > Sure. Glad that I asked :-) > > Well ..., if we have an EXPR_FVALUE which type is a bitfield, yes > for sure there would be a bug. But I have no reason to belive there > is such bug and since checking the ctype is more costly than checking > the expr->type, I think it's best so. > > Note, I find this check already annoying and hackish if not worse. > Yes, from your other email said there is a bug some where else in sparse yet. (the text I quote you). Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 21, 2017 at 6:06 PM, Christopher Li <sparse@chrisli.org> wrote: > > I am referenitng to your email reply to Dibyendu: > > =========================== > It's a very surprising bug. It's not a linearization or > an optimization bug as the AST is already wrong. > With a simpler test case, like: > struct s { > char a:4; > char b:4; > }; > > int foo(void) > { > struct s x = { .a = 2, .b = 4 }; > > return x.b; > } > > you can see that the linearization produce correct > code for the initializer. > You can also see that the return statement to be > linearized is something like > STMT_RETURN > ret_value: EXPR_VALUE (value = 2) It's precisely this bug that this patch fixes. > > Yes, from your other email said there is a bug some where else > in sparse yet. (the text I quote you). -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 21, 2017 at 12:14 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Mon, Aug 21, 2017 at 6:06 PM, Christopher Li <sparse@chrisli.org> wrote: >> >> I am referenitng to your email reply to Dibyendu: >> >> =========================== >> It's a very surprising bug. It's not a linearization or >> an optimization bug as the AST is already wrong. >> With a simpler test case, like: >> struct s { >> char a:4; >> char b:4; >> }; >> >> int foo(void) >> { >> struct s x = { .a = 2, .b = 4 }; >> >> return x.b; >> } >> >> you can see that the linearization produce correct >> code for the initializer. >> You can also see that the return statement to be >> linearized is something like >> STMT_RETURN >> ret_value: EXPR_VALUE (value = 2) > > > It's precisely this bug that this patch fixes. Oh, I see. I misunderstand what bug the patch fix. Let me go back to read it again. Sorry about that. Thanks for the explain. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/expand.c b/expand.c index f436b5b50..d44cfac73 100644 --- a/expand.c +++ b/expand.c @@ -644,6 +644,8 @@ static int expand_dereference(struct expression *expr) if (value) { /* FIXME! We should check that the size is right! */ if (value->type == EXPR_VALUE) { + if (is_bitfield_type(value->ctype)) + return UNSAFE; expr->type = EXPR_VALUE; expr->value = value->value; expr->taint = 0; diff --git a/validation/bitfield-expand-deref.c b/validation/bitfield-expand-deref.c new file mode 100644 index 000000000..58d1fe176 --- /dev/null +++ b/validation/bitfield-expand-deref.c @@ -0,0 +1,27 @@ +struct s { + int a:8; + int b:8; +}; + +int foo(void) +{ + struct s x = { .a = 12, .b = 34, }; + + return x.b; +} + +int bar(int a) +{ + struct s x = { .a = 12, .b = a, }; + + return x.b; +} + +/* + * check-name: bitfield expand deref + * check-command: test-linearize -Wno-decl $file + + * check-output-ignore + * check-output-excludes: ret\..*\$12 + * check-output-contains: ret\..*\$34 + */
During the expansion of a dereference, it's if the initializer which corrrespond to the offset we're interested is a constant. In which case this dereference can be avoided and the value given in the initializer can be used instead. However, it's not enough to check for the offset since for bitfields several are placed at the same offset. Currently, the first initializer matching the offset is selected. Fix this by refusing such expansion if the constant value correspond to a bitfield. Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- expand.c | 2 ++ validation/bitfield-expand-deref.c | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 validation/bitfield-expand-deref.c