| Message ID | 1400534662-5729-1-git-send-email-rickard_strandqvist@spectrumdigital.se |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 05/20/2014 01:24 AM, Rickard Strandqvist wrote: > There is otherwise a risk of a possible null pointer dereference. > Was largely found by using a static code analysis program called cppcheck. > Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> > --- > drivers/isdn/hisax/l3dss1.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > diff --git a/drivers/isdn/hisax/l3dss1.c b/drivers/isdn/hisax/l3dss1.c > index cda7006..9ea7377 100644 > --- a/drivers/isdn/hisax/l3dss1.c > +++ b/drivers/isdn/hisax/l3dss1.c > @@ -2203,13 +2203,14 @@ static int l3dss1_cmd_global(struct PStack *st, isdn_ctrl *ic) > memcpy(p, ic->parm.dss1_io.data, ic->parm.dss1_io.datalen); /* copy data */ > l = (p - temp) + ic->parm.dss1_io.datalen; /* total length */ > > - if (ic->parm.dss1_io.timeout > 0) > - if (!(pc = dss1_new_l3_process(st, -1))) > - { free_invoke_id(st, id); > + if (ic->parm.dss1_io.timeout > 0) { > + if (!(pc = dss1_new_l3_process(st, -1))) { Assignments shouldn't be put into *if* statement. If you did run the patch thru scripts/checkpatch.pl, it would have told you. > + free_invoke_id(st, id); > return (-2); Parens not needed. Could as well fix it here... > } > - pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */ > - pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */ > + pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */ > + pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */ > + } WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei
I did not put the assignment in the if statement. Is it meant for me
to change all the code around the parts I fix?
There are many assignments in if in the l3dss1_cmd_global() funktion.
It's late and I'm tired, but I still can not see which of these pairs
of features that I can remove ...?
if (ic->parm.dss1_io.timeout > 0) {
if (!(pc = dss1_new_l3_process(st, -1))) {
free_invoke_id(st, id);
return (-2);
}
pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */
pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */
}
Best regards
Rickard Strandqvist
2014-05-19 23:26 GMT+02:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> On 05/20/2014 01:24 AM, Rickard Strandqvist wrote:
>
>> There is otherwise a risk of a possible null pointer dereference.
>
>
>> Was largely found by using a static code analysis program called cppcheck.
>
>
>> Signed-off-by: Rickard Strandqvist
>> <rickard_strandqvist@spectrumdigital.se>
>> ---
>> drivers/isdn/hisax/l3dss1.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>
>
>> diff --git a/drivers/isdn/hisax/l3dss1.c b/drivers/isdn/hisax/l3dss1.c
>> index cda7006..9ea7377 100644
>> --- a/drivers/isdn/hisax/l3dss1.c
>> +++ b/drivers/isdn/hisax/l3dss1.c
>> @@ -2203,13 +2203,14 @@ static int l3dss1_cmd_global(struct PStack *st,
>> isdn_ctrl *ic)
>> memcpy(p, ic->parm.dss1_io.data,
>> ic->parm.dss1_io.datalen); /* copy data */
>> l = (p - temp) + ic->parm.dss1_io.datalen; /*
>> total length */
>>
>> - if (ic->parm.dss1_io.timeout > 0)
>> - if (!(pc = dss1_new_l3_process(st, -1)))
>> - { free_invoke_id(st, id);
>> + if (ic->parm.dss1_io.timeout > 0) {
>> + if (!(pc = dss1_new_l3_process(st, -1))) {
>
>
> Assignments shouldn't be put into *if* statement. If you did run the
> patch thru scripts/checkpatch.pl, it would have told you.
>
>
>> + free_invoke_id(st, id);
>> return (-2);
>
>
> Parens not needed. Could as well fix it here...
>
>
>> }
>> - pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /*
>> remember id */
>> - pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and
>> procedure */
>> + pc->prot.dss1.ll_id =
>> ic->parm.dss1_io.ll_id; /* remember id */
>> + pc->prot.dss1.proc =
>> ic->parm.dss1_io.proc; /* and procedure */
>> + }
>
>
> WBR, Sergei
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 20-05-2014 1:56, Rickard Strandqvist wrote: > Hi Sergei > I did not put the assignment in the if statement. You didn't but you changed the line it's on (which wasn't required BTW). > Is it meant for me > to change all the code around the parts I fix? No, the patch just needs to be checkpatch.pl-clean. > There are many assignments in if in the l3dss1_cmd_global() funktion. You can leave them alone for now. You can also leave alone the line I was referring to. > It's late and I'm tired, but I still can not see which of these pairs > of features that I can remove ...? > if (ic->parm.dss1_io.timeout > 0) { > if (!(pc = dss1_new_l3_process(st, -1))) { > free_invoke_id(st, id); > return (-2); > } > pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */ > pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */ > } I didn't quite get the question, if you were asking me. > Best regards > Rickard Strandqvist WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/drivers/isdn/hisax/l3dss1.c b/drivers/isdn/hisax/l3dss1.c index cda7006..9ea7377 100644 --- a/drivers/isdn/hisax/l3dss1.c +++ b/drivers/isdn/hisax/l3dss1.c @@ -2203,13 +2203,14 @@ static int l3dss1_cmd_global(struct PStack *st, isdn_ctrl *ic) memcpy(p, ic->parm.dss1_io.data, ic->parm.dss1_io.datalen); /* copy data */ l = (p - temp) + ic->parm.dss1_io.datalen; /* total length */ - if (ic->parm.dss1_io.timeout > 0) - if (!(pc = dss1_new_l3_process(st, -1))) - { free_invoke_id(st, id); + if (ic->parm.dss1_io.timeout > 0) { + if (!(pc = dss1_new_l3_process(st, -1))) { + free_invoke_id(st, id); return (-2); } - pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */ - pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */ + pc->prot.dss1.ll_id = ic->parm.dss1_io.ll_id; /* remember id */ + pc->prot.dss1.proc = ic->parm.dss1_io.proc; /* and procedure */ + } if (!(skb = l3_alloc_skb(l))) { free_invoke_id(st, id);
There is otherwise a risk of a possible null pointer dereference. Was largely found by using a static code analysis program called cppcheck. Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> --- drivers/isdn/hisax/l3dss1.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)