-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use-after-free in AsyncWebServerRequest::_removeNotInterestingHeaders #951
Comments
This branch has all my pull requests:
https://github.com/Depau/ESPAsyncWebServer/tree/wi-se-patches
…On Tue, May 25, 2021, 09:12 zekageri ***@***.***> wrote:
Do you have a fork to try with the fixes?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#951 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIZCWIV32N3VR5JSBU5LEDTPNEWRANCNFSM4YX3HCKA>
.
|
Thanks. I found one before but I think it wasnt that one. I will try it. Iam experiencing random crashes on my esp and it seems it is because this linked list problem. I cant debug because its remote and I cant put serial to it |
If you'd like to run your ESP application on a Linux computer, I wrote a bunch of stubs to do so: https://github.com/Depau/wi-se-sw/tree/main/fakeesp This is what allowed me to run this library with Valgrind/GDB and find the issues. YMMV though, and I don't have time to provide any supports on them. Feel free to do whatever you want with the code, if you have any improvements lmk. |
Thank you for the help. Your version of the library seems pretty stable. It runs almost 4 days straight now. |
[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions. |
Unstale, still waiting for this to be reviewed |
[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future. |
@depau can you please pull latest |
[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions. |
Unstale |
[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future. |
Is this bug fix going to be merged? |
[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions. |
Is this bugfix already integrated? |
[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future. |
No |
@me-no-dev Is it planned to integrate this fix ? (#952). We are currently trying to port our software (WLED) to the new MCU's, and this one seems to cause troubles on -C3 and (maybe) -S2. Another problem we run into on -S3 and -S2 is described in #1210 - "[E][vfs_api.cpp:29] open(): myFile.txt does not start with /" Would be great if these issues could be solved. |
I opened a PR against the wi-se-patches branch here depau#1 to align it with its upstream it's not much but at least if you're using a recent version of esp-idf with platformio you can simply point it to the fixed and up-to-date version with
|
[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions. |
Upgrade the list structure to support quick insertion, and fix a use-after-free in _removeNotInterestingHeaders. This bug has been reported and "fixed" many times upstream, but several of those fixes seem to be bad. See me-no-dev/ESPAsyncWebServer me-no-dev#951, me-no-dev#837
Upgrade the list structure to support quick insertion, and fix a use-after-free in _removeNotInterestingHeaders. This bug has been reported and "fixed" many times upstream, but several of those fixes seem to be bad. See me-no-dev/ESPAsyncWebServer me-no-dev#951, me-no-dev#837
replace in WebRequest.cpp void AsyncWebServerRequest::_removeNotInterestingHeaders(){
if (_interestingHeaders.containsIgnoreCase("ANY")) return; // nothing to do
StringArray *a = &_interestingHeaders;
auto cmpr = LinkedList<AsyncWebHeader*>::Predicate{[&a](AsyncWebHeader* h) {
return !a->containsIgnoreCase(h->name().c_str());
}};
while (_headers.remove_first(cmpr)){}
} or void AsyncWebServerRequest::_removeNotInterestingHeaders(){
if (_interestingHeaders.containsIgnoreCase("ANY")) return; // nothing to do
StringArray *a = &_interestingHeaders;
auto cmpr = LinkedList<AsyncWebHeader*>::Predicate{[&a](AsyncWebHeader* h) {
return !a->containsIgnoreCase(h->name().c_str());
}};
_headers.remove_all(cmpr);
} and add metod to class LinkedList in StringArray.h bool remove_all(Predicate predicate){
auto it = _root;
auto pit = _root;
bool flag = false;
while (it) {
if(predicate(it->value())){
auto d = it;
if (it == _root) {
_root = _root->next;
pit = _root;
it = _root;
} else {
pit->next = it->next;
it = pit->next;
}
if (_onRemove) {
_onRemove(d->value());
}
delete d;
flag = true;
} else {
pit = it;
it = it->next;
}
}
return flag;
}
|
[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future. |
Hi,
While running my code with Valgrind in an effort to troubleshoot memory-corruption issues I noticed there's a use-after-free error in this library's code.
Here the function iterates over a linked list of headers, and removes not-interesting ones from the list.
https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/WebRequest.cpp#L183-L186
Here, the linked list's remove method correctly updates the next pointer and frees the removed item:
https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/StringArray.h#L120-L132
However, if an item is deleted during iteration, the iterator still holds a reference to its memory, and it still uses it to retrieve the next pointer:
https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/StringArray.h#L53
While this works in most cases and particularly on the ESP MCUs, on my testing setup I get a nice SIGSEGV.
A potential fix could be retrieving the
next
pointer ahead of time and storing it in the iterator. Then inoperator++
replace it with the "next-next" pointer and return the storednext
pointer. This should make deleting the current item safe.(If you're wondering how the hell I'm running it with valgrind, I stubbed all the Arduino calls and patched ESPAsyncTCP to use the POSIX socket API, and I'm running my application as a regular Linux program)
The text was updated successfully, but these errors were encountered: