Re: Coverity issues in linux-wpan

[Varka Bhadram <varkabhadram@gmail.com>]

Hi,

On 06/09/2015 01:40 AM, Stefan Schmidt wrote:

> Hello.
>
> On 08/06/15 19:04, Alexander Aring wrote:
>> On Mon, Jun 08, 2015 at 03:05:54PM +0200, Stefan Schmidt wrote:
>>> Hello.
>>>
>>> I gave the currently found coverity issues in the linux-wpan area a 
>>> look.
>>> The analyzing builds are not tracking bluetooth-next but go with 
>>> Linus tree
>>> and get updated from time to time.
>>>
>>> As I'm not sure if all people here have access to this (you should 
>>> request
>>> it, its useful) I will cite the reported issues here with some context.
>>>
>>>  From what I can see 5 issues have been detected. Two in mac802154 
>>> and three
>>> in our drivers (cc2520 and mrf24j40).
>>>
>>> mrf24j40 (CID: 1226982, 1226983)
>>> o Return value with error codes gets overridden before returning.
>>> o I have a patch for that which I will send right after this mail.
>>>
>>> cc2520 (CID: 1230469)
>>> o Unchecked return value.
>>> o Should we check here as we do in the other cases or not?
>>>
>>> 622 if (filt->pan_coord)
>>> CID 1230469 (#1 of 2): Unchecked return value (CHECKED_RETURN)
>>> <https://scan5.coverity.com:8443/defectInstanceId=22519459&fileInstanceId=73901097&mergedDefectId=1230469> 
>>>
>>> 623 cc2520_write_register(priv, CC2520_FRMFILT0, 0x02);
>>> 624 else
>>> CID 1230469 (#2 of 2): Unchecked return value (CHECKED_RETURN)10.
>>> check_return: Calling cc2520_write_register without checking return 
>>> value
>>> (as is done elsewhere 21 out of 24 times).
>>> 625 cc2520_write_register(priv, CC2520_FRMFILT0, 0x00);
>>>
>>> mac802154/iface.c (CID: 1268751)
>>> o WARN_ON clears the parameter so we can't check for it afterwards 
>>> to hit
>>> the error path
>>>
>>> 147 if (!local->open_count) {
>>> 148 res = drv_start(local);
>>> cond_const: Condition res, taking false branch. Now the value of res is
>>> equal to 0.
>>> 149 WARN_ON(res);
>>> const: At condition res, the value of res must be equal to
>>> 0.dead_error_condition: The condition res cannot be true.
>>> 150 if (res)
>>> CID 1268751 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
>>> Execution cannot reach this statement: goto err;.
>>> 151 goto err;
>>> 152        }
>>>
>> Simple remove the WARN_ON or do WARN_ON(1) here, I would remove it.
>>
>> I remember this section [0]. I leave the old beheavior which was not
>> like wireless code. Nevertheless I had some bad feeling when I saw this
>> and doing some code around. This behaviour was always there.
>>
>> Comparing with wireless code, see [2].
> OK, this makes more sense now. I send a patch removing it. That way it 
> works like we intend it.
>
>>> mac802154/rx.c (CID: 1260107)
>>> o Coverity thinks hdr.seq might be used unitiliazed in the pr_debug 
>>> from
>>> ieee802154_parse_frame_start (line 149)
>>> o Setting hdr.seq = 0; here would not hurt but I still wonder how 
>>> coverity
>>> comes to think that as we pull the first three byte of the header 
>>> before
>>> which include fc and seq.
>>>
>>> 199 struct ieee802154_sub_if_data *sdata;
>>> 1. var_decl: Declaring variable hdr without initializer.
>>> 200 struct ieee802154_hdr hdr;
>>> 201
>>> CID 1260107 (#1 of 1): Uninitialized scalar variable (UNINIT)2.
>>> uninit_use_in_call: Using uninitialized value hdr.seq when calling
>>> ieee802154_parse_frame_start.
>>> <https://scan5.coverity.com:8443/eventId=22526690-1&modelId=22526690-0&fileInstanceId=73892728&filePath=%2Fnet%2Fmac802154%2Frx.c&fileStart=136&fileEnd=192> 
>>>
>>> 202 ret = ieee802154_parse_frame_start(skb, &hdr);
>>>
>> I think the hdr.seq should be filled by doing [1].
>>
> Agreed. That is also what I think where the seq should be filled from. 
> No idea why coverity gets this wrong. For now I consider this a false 
> positive and no real issue for us.
>
> That leaves only the cc2520 report open. Hopefully Varka has some 
> comment on this. We can either assume that ignoring the return in 
> these cases is fine or we add the error handling for them.

mhhh.. Its better to have the error handling at this place.

Please send the patch.

-- 
Varka Bhadram