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

samples: net: azure: Avoid negative array index write #27192

Conversation

jukkar
Copy link
Member

@jukkar jukkar commented Jul 28, 2020

If the mqtt_read_publish_payload() returns <0 value, then do
not use that value when accessing the data array.

Fixes: #25775
Coverity-CID: 210075

Signed-off-by: Jukka Rissanen [email protected]

If the mqtt_read_publish_payload() returns <0 value, then do
not use that value when accessing the data array.

Fixes: zephyrproject-rtos#25775
Coverity-CID: 210075

Signed-off-by: Jukka Rissanen <[email protected]>
@@ -249,8 +249,9 @@ static void mqtt_event_handler(struct mqtt_client *const client,
data,
len >= sizeof(data) - 1 ?
sizeof(data) - 1 : len);
if (bytes_read < 0 && bytes_read != -EAGAIN) {
LOG_ERR("failure to read payload");
if (bytes_read < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mqtt_read_publish_payload_blocking should be used here instead. mqtt_read_publish_payload is non-blocking function, so returning -EAGAIN is not really an error (but the design of this loop isn't great since it'll keep spinning in that case apart from the issue detected by coverity). With the blocking function, it should be safe to abort on any error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds better indeed. I am not able to test these kind of changes right now so need to postpone this PR a bit.

@jukkar jukkar added the DNM This PR should not be merged (Do Not Merge) label Aug 3, 2020
@dleach02
Copy link
Member

Can we pick this back up? It will help with the release metrics.

@jukkar
Copy link
Member Author

jukkar commented Sep 15, 2020

Can we pick this back up? It will help with the release metrics.

I am not able to work on this right now, but I do not object if someone is able to.

@jenmwms jenmwms added bug The issue is a bug, or the PR is fixing a bug Coverity A Coverity detected issue or its fix labels Sep 15, 2020
@github-actions
Copy link

This pull request 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 pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

Copy link
Contributor

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jukkar, @rlubos: What's the status of this re: inclusion in 2.5.0?

@carlescufi
Copy link
Member

carlescufi commented Feb 9, 2021

@rlubos do you have the means to test this patch? Otherwise I suggest we accept the fix.

@rlubos
Copy link
Contributor

rlubos commented Feb 10, 2021

@rlubos do you have the means to test this patch? Otherwise I suggest we accept the fix.

Currently not, my Azure trial has expired.

@github-actions
Copy link

This pull request 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 pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Apr 12, 2021
@github-actions github-actions bot closed this Apr 27, 2021
@jukkar jukkar deleted the bug-25775-neg-index-azure-sample branch February 29, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking area: Samples Samples bug The issue is a bug, or the PR is fixing a bug Coverity A Coverity detected issue or its fix DNM This PR should not be merged (Do Not Merge) Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Coverity CID :210075] Negative array index write in samples/net/cloud/mqtt_azure/src/main.c
6 participants