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
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
