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

esp_http_client: fix the residual data issue and potential out-of-bounds access (IDFGH-10530) #11775

Closed
wants to merge 2 commits into from

Conversation

hwqchi
Copy link
Contributor

@hwqchi hwqchi commented Jun 29, 2023

  1. Fix potential out-of-bounds access when calling strlen(local_response_buffer) if content_length is greater than or equal to the length of local_response_buffer due to missing the terminator \0 at the last character position.
  2. Fix the residual data issue when the previous request is longer than the subsequent request while outputting the local_response_buffer for each request in the http_rest_with_url() function.

…nds access

1. Fix potential out-of-bounds access when calling `strlen(local_response_buffer)` if `content_length` is greater than or equal to the length of `local_response_buffer` due to missing the terminator `\0` at the last character position.
2. Fix the residual data issue when the previous request is longer than the subsequent request while outputting the `local_response_buffer` for each request in the `http_rest_with_url()` function.
@espressif-bot espressif-bot added the Status: Opened Issue is new label Jun 29, 2023
@github-actions github-actions bot changed the title esp_http_client: fix the residual data issue and potential out-of-bounds access esp_http_client: fix the residual data issue and potential out-of-bounds access (IDFGH-10530) Jun 29, 2023
@hmalpani
Copy link
Contributor

hmalpani commented Jul 3, 2023

Hello @hwqchi
Thanks for adding the fix.

@hmalpani
Copy link
Contributor

hmalpani commented Jul 3, 2023

sha=5ed88da1d37473dfaca32039004ccc1cf7d331d0

@hmalpani hmalpani added the PR-Sync-Merge Pull request sync as merge commit label Jul 3, 2023
@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Jul 3, 2023
@hmalpani
Copy link
Contributor

hmalpani commented Jul 6, 2023

Hello @hwqchi
After some discussions internally, I have updated the changes in this PR. Can you please try the attached changes and verify their sanity?
Also, it will be helpful if you can share some test cases that can be added to the example to test these cases.

Thanks!
IDFGH-10530.txt

@hmalpani
Copy link
Contributor

Hello @hwqchi
Any updates on the patch I shared earlier?

@AdityaHPatwardhan AdityaHPatwardhan added the Awaiting Response awaiting a response from the author label Jul 14, 2023
@hwqchi
Copy link
Contributor Author

hwqchi commented Jul 15, 2023

Hey @hmalpani,

I'm really sorry for taking so long to reply to your feedback on my pull request. Life has been quite busy lately, and I couldn't find the time to respond promptly.

First of all, I want to thank you from the bottom of my heart for going through my code and providing valuable suggestions. I truly appreciate the effort you put into reviewing it.

I also want to let you know that I've tried out the changes you suggested and tested the modified code, and I'm happy to report that everything is working perfectly now. Your modifications have effectively resolved the issues, and I found your solution much better than my previous attempt with "memset(0)".

Once again, I apologize for the inconvenience caused by my delayed reply. Your contribution and guidance mean a lot to me, and I promise to be more prompt and proactive in the future.

By the way, I'll be running some additional tests on strict cases in the coming days, just to ensure everything is working flawlessly.

Thanks again for your understanding and support.

All the best,
@hwqchi

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: In Progress Work is in progress labels Jul 24, 2023
@mahavirj
Copy link
Member

Merged with ebc118b

@mahavirj mahavirj closed this Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Response awaiting a response from the author PR-Sync-Merge Pull request sync as merge commit Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants