Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Coverity CID :203443]Memory - corruptions in /subsys/bluetooth/host/rfcomm.c #18386

Closed
aasthagr opened this issue Aug 17, 2019 · 10 comments · Fixed by #35706
Closed

[Coverity CID :203443]Memory - corruptions in /subsys/bluetooth/host/rfcomm.c #18386

aasthagr opened this issue Aug 17, 2019 · 10 comments · Fixed by #35706
Assignees
Labels
area: Bluetooth bug The issue is a bug, or the PR is fixing a bug Coverity A Coverity detected issue or its fix priority: low Low impact/importance bug

Comments

@aasthagr
Copy link
Collaborator

aasthagr commented Aug 17, 2019

Static code scan issues seen in File: /subsys/bluetooth/host/rfcomm.c
Category: Memory - corruptions
Function: bt_rfcomm_dlc_send
Component: Bluetooth
CID: 203443
Please fix or provide comments to square it off in coverity in the link: https://scan9.coverity.com/reports.htm#v32951/p12996
https://scan9.coverity.com/reports.htm#v34747/p12996/

@aasthagr aasthagr added area: Bluetooth bug The issue is a bug, or the PR is fixing a bug Coverity A Coverity detected issue or its fix labels Aug 17, 2019
@galak galak added the priority: medium Medium impact/importance bug label Aug 18, 2019
@jhedberg jhedberg added priority: low Low impact/importance bug and removed priority: medium Medium impact/importance bug labels Aug 18, 2019
@jhedberg
Copy link
Member

There are several things that look messed up with this code. I don't have time to go digging through the RFCOMM spec to figure out what exactly is the right thing. Maybe this is the final straw and we should just remove BR/EDR support from the tree. Downgrading to low priority however since this is considered experimental code.

@joerchan
Copy link
Contributor

joerchan commented Jan 3, 2020

@aasthagr Could you possibly check if this issue has been resolved with the fix that was recently submitted to RFCOMM?

@joerchan
Copy link
Contributor

aasthagr Could you possibly check if this issue has been resolved with the fix that was recently submitted to RFCOMM?

Answering my own question. No it does not.

@nashif
Copy link
Member

nashif commented Mar 11, 2020

this is marked fixed in coverity with comment

"Source code is not available because it has been purged."

@nashif nashif closed this as completed Mar 11, 2020
@nashif
Copy link
Member

nashif commented Mar 5, 2021

the issue is still there in coverity

image

@nashif nashif reopened this Mar 5, 2021
@joerchan
Copy link
Contributor

joerchan commented Mar 5, 2021

This is intentional, rfcomm has a variable length length field, which is confusing when we cast the uint8_t field to an uint16_t field instead, but the extra byte has been pushed to the net_buf.

@jhedberg I think it would be slighly more correct to make the rfcomm header exclude the length field, and then either push or pull uint8_t or uint16_t from net_buf instead, do you think it is worth it to change it like this?
I don't have any BR/EDR stuff to test any change I would make, so would have to rely on review only I think.

@github-actions
Copy link

github-actions bot commented May 5, 2021

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label May 5, 2021
@nashif nashif removed the Stale label May 5, 2021
@galak galak added this to the v2.6.0 milestone May 11, 2021
@carlescufi carlescufi assigned MaureenHelm and unassigned jhedberg and joerchan May 20, 2021
@carlescufi
Copy link
Member

@MaureenHelm assigning this to you since @MarkWangChinese is not yet part of the org, and this is BR/EDR with which you've shown an interest in keeping alive.

@MaureenHelm
Copy link
Member

@MarkWangChinese can you take a look this week?

@MarkWangChinese
Copy link
Contributor

I created one PR #35706

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth bug The issue is a bug, or the PR is fixing a bug Coverity A Coverity detected issue or its fix priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants