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
asiohiper.cpp: event_cb can be called with deallocated fdp pointer, causing crash #5090
Comments
The asiohiper.cpp example is known to be buggy and it has a prominent warning at the top of the file: There seems to be very little interest or ability to fix it. I know I personally just get confused by asio so I'm not the man for the job. Unfortunately, I would say that our best course of action right now - unless we get help to actually fix the example - is to simply remove the example source from the tarballs and web site. |
Hey Daniel -
Thanks for the quick response. So I have experience with asio because we
use it in various parts of our app, and I actually really like it - but
maybe I am the minority. Anyway, our modified asiohiper.cpp code - not
just the snippet I put in the bug but the rest of the supporting changes -
solves the issue and is working out great for us. Let me check with my
company about just submitting that whole thing.
I like the model of being able to use asio and have the number of threads
be tunable independently of the number of sockets. That is really nice.
…-Andy
On Thu, Mar 12, 2020 at 6:21 AM Daniel Stenberg ***@***.***> wrote:
The asiohiper.cpp example is known to be buggy and it has prominent
warning at the top of the file:
*WARNING: This example program is known to have flaws:*
There seems to be very little interest or ability to fix it. I know I
personally just get confused by asio so I'm not the man for the job.
Unfortunately, I would say that our best course of action right now -
unless we get help to actually fix the example - is to simply remove the
exampled from the tarballs and web site.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5090 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOZ2DCDL3K4IA3JB4IJDULTRHDOVTANCNFSM4LGMBE2Q>
.
|
I will proceed and remove asiohiper.cpp before the next release unless we hear something else soon. I don't like how it causes problems for people and I think we''re better off without the example rather than shipping something that crashes occasionally when used. |
Hey Daniel -
Yeah this slipped my mind for a while, we have been the in the throws of a
big release at my company which wraps up soon. So our fix is looking good
- our asio based curl code has been stable for months after I made the fix.
If I were able to apply the fix to the sample code but make it use c++
features, would that be useful or are you trying to keep everything in
straight c?
…-Andy
On Thu, Apr 2, 2020 at 3:24 PM Daniel Stenberg ***@***.***> wrote:
I will proceed and remove asiohiper.cpp before the next release unless we
hear something else soon. I don't like how it causes problems for people
and I think we''re better off without the example rather than shipping
something that crashes occasionally when used.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5090 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOZ2DCBKTYPLIIPZDNRLFRLRKUGCPANCNFSM4LGMBE2Q>
.
|
This is a cpp file, so C++ is fine! ;-) |
Oops, sorry not enough coffee yet I was mixing it up with other files.
…On Fri, Apr 3, 2020 at 7:45 AM Marcel Raad ***@***.***> wrote:
This is a cpp file, so C++ is fine! ;-)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5090 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOZ2DCEOC276MZ2G7UC4JR3RKXZCNANCNFSM4LGMBE2Q>
.
|
This example has repeatedly been reported to contain bugs, and as users copy and paste code from this into production, I now deem it better to not provide the example at all. Closes #5090
@andrewoliverhall though asiohiper has now been removed if the changes you made are still available please let us know, we could really use a working example of it. Thanks. |
I did this
I am using the example code in asiohiper.cpp in a large application doing lots of http requests. We also use asio in other contexts. Often we run on osx using xcode "address sanitizer" and it flagged an issue in the asiohiper.cpp code, specifically in "event_cb". This issue was causing intermittent crashes in our application because of memory corruption.
The problem is that event_cb is passed as a function callback for async_read_some or async_write_some, where the boost::bind statement is capturing the pointer value of fdp as a parameter to event_cb. However, sometime between when async_read_some or async_write_some is invoked and when event_cb is called, the fdp pointer is deallocated via close_socket. This leads to an uninitialized memory read at the start of event_cb where it dereferences fdp.
My solution is to use a pattern I use elsewhere with asynchronous programming and boost::asio: weak_ptr. I created a struct such as:
socket_map is changed to be a map from curl_socket_t to a std::shared_ptr.
Then I changed event_cb like this:
There were various other changes to support this in the file. I like the weak_ptr technique but perhaps there is a more incremental way to solve the issue.
I expected the following
No crashes in my libcurl code.
curl/libcurl version
7.56.1. ( but this is a bug in the latest asiohiper.cpp on github, not a bug in libcurl exactly )
[curl -V output]
operating system
The text was updated successfully, but these errors were encountered: