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

Handle event_cb being called after the socket is closed. #1318

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@edkimmel

edkimmel commented Mar 8, 2017

I used asiohiper as a starting point for networking in one of my applications.

This pull request shows one of the big differences between the example and my code, which resolved a bad access inside of event_cb, caused by event_cb being called after the socket was closed.

It checks that the curl_socket_t is still in the socket map at the very beginning of event_cb.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 8, 2017

@edkimmel, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lijoantony, @bagder and @Aulddays to be potential reviewers.

mention-bot commented Mar 8, 2017

@edkimmel, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lijoantony, @bagder and @Aulddays to be potential reviewers.

@bagder bagder added the documentation label Mar 9, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 9, 2017

Member

ping @Aulddays, do you have any feedback or comments on this PR? (I know nothing about boost)

Member

bagder commented Mar 9, 2017

ping @Aulddays, do you have any feedback or comments on this PR? (I know nothing about boost)

@Aulddays

This comment has been minimized.

Show comment
Hide comment
@Aulddays

Aulddays Mar 23, 2017

Contributor

Sorry, I was away for a few days... I'll take a look ASAP.

Contributor

Aulddays commented Mar 23, 2017

Sorry, I was away for a few days... I'll take a look ASAP.

@Aulddays

This comment has been minimized.

Show comment
Hide comment
@Aulddays

Aulddays Mar 23, 2017

Contributor

The solution is to check whether the socket is still in the global map to avoid access after close. I think that as long as curl_socket_t is an alias for the underlying file descriptor for the socket, which is an int value, may be it's possible that one socket was closed and soon another new socket just reuse the same file descriptor value, resulting in that the int value can be found in the map but pointing to another socket connection.
I'm not so sure whether this (socket file descriptor value being reused in a short time) would happen. If would not, then I think the solution is good. @bagder @edkimmel

Contributor

Aulddays commented Mar 23, 2017

The solution is to check whether the socket is still in the global map to avoid access after close. I think that as long as curl_socket_t is an alias for the underlying file descriptor for the socket, which is an int value, may be it's possible that one socket was closed and soon another new socket just reuse the same file descriptor value, resulting in that the int value can be found in the map but pointing to another socket connection.
I'm not so sure whether this (socket file descriptor value being reused in a short time) would happen. If would not, then I think the solution is good. @bagder @edkimmel

@MarcelRaad

Nit: socket_map.find(s) is called three times now. One of them (line 202) is redundant now and the other one (line 204) could just use the result of the first one (line 179).

Other than that, this looks correct.

@MarcelRaad

This comment has been minimized.

Show comment
Hide comment
@MarcelRaad

MarcelRaad Mar 23, 2017

Member

I'm not so sure whether this (socket file descriptor value being reused in a short time) would happen. If would not, then I think the solution is good.

As there's only one io_service worker thread (which is the main thread), I don't think this can happen. The asio and curl callbacks are always called in the same thread.

Member

MarcelRaad commented Mar 23, 2017

I'm not so sure whether this (socket file descriptor value being reused in a short time) would happen. If would not, then I think the solution is good.

As there's only one io_service worker thread (which is the main thread), I don't think this can happen. The asio and curl callbacks are always called in the same thread.

@edkimmel

This comment has been minimized.

Show comment
Hide comment
@edkimmel

edkimmel Mar 23, 2017

The find on 204 can't reuse the one from 179. The sockets can be closed in between there, on the curl_multi calls.

At most, I can reduce it to two.

edkimmel commented Mar 23, 2017

The find on 204 can't reuse the one from 179. The sockets can be closed in between there, on the curl_multi calls.

At most, I can reduce it to two.

@MarcelRaad

This comment has been minimized.

Show comment
Hide comment
@MarcelRaad

MarcelRaad Mar 23, 2017

Member

The find on 204 can't reuse the one from 179. The sockets can be closed in between there, on the curl_multi calls.

At most, I can reduce it to two.

Ah, you're right, sorry! No, then it's good as is. If nobody's faster than me, I'll merge it as soon as I'm at home in a few hours. Thanks a lot!

Member

MarcelRaad commented Mar 23, 2017

The find on 204 can't reuse the one from 179. The sockets can be closed in between there, on the curl_multi calls.

At most, I can reduce it to two.

Ah, you're right, sorry! No, then it's good as is. If nobody's faster than me, I'll merge it as soon as I'm at home in a few hours. Thanks a lot!

@Aulddays

This comment has been minimized.

Show comment
Hide comment
@Aulddays

Aulddays Mar 24, 2017

Contributor

As there's only one io_service worker thread (which is the main thread), I don't think this can happen. The asio and curl callbacks are always called in the same thread.

Single thread assures callbacks are called one by one in serial, but it does not guarantee the ORDER. As long as the socket_map are checked at entrance of event_cb(), it implies that the socket may have been closed before callback of event_cb(). As long as the socket may have been closed, it is possible that it also may have been reused. So single thread assures the creation/reuse of the new socket would not happen at the same time, but not guarantee the new socket did not happen before call back of event_cb().

Contributor

Aulddays commented Mar 24, 2017

As there's only one io_service worker thread (which is the main thread), I don't think this can happen. The asio and curl callbacks are always called in the same thread.

Single thread assures callbacks are called one by one in serial, but it does not guarantee the ORDER. As long as the socket_map are checked at entrance of event_cb(), it implies that the socket may have been closed before callback of event_cb(). As long as the socket may have been closed, it is possible that it also may have been reused. So single thread assures the creation/reuse of the new socket would not happen at the same time, but not guarantee the new socket did not happen before call back of event_cb().

@MarcelRaad

This comment has been minimized.

Show comment
Hide comment
@MarcelRaad

MarcelRaad Mar 24, 2017

Member

@Aulddays Thanks, now I see what you mean. I don't think there's anything easy that can be done about that which doesn't bloat the example code, is there? Even the pointers could be reused.

Member

MarcelRaad commented Mar 24, 2017

@Aulddays Thanks, now I see what you mean. I don't think there's anything easy that can be done about that which doesn't bloat the example code, is there? Even the pointers could be reused.

@Aulddays

This comment has been minimized.

Show comment
Hide comment
@Aulddays

Aulddays Mar 25, 2017

Contributor

Yeah, I agree. One way I have in mind so far is something like a reference number or a status code, stored in a pointer. Callback order is not guaranteed, but it will get called eventually. Quite a heavy way for an example.

Contributor

Aulddays commented Mar 25, 2017

Yeah, I agree. One way I have in mind so far is something like a reference number or a status code, stored in a pointer. Callback order is not guaranteed, but it will get called eventually. Quite a heavy way for an example.

@edkimmel

This comment has been minimized.

Show comment
Hide comment
@edkimmel

edkimmel Apr 13, 2017

I can confirm that a socket can be closed and reused and cause a bit of chaos.

Since my project isn't just an example, what ideas do you have to handle this?

edkimmel commented Apr 13, 2017

I can confirm that a socket can be closed and reused and cause a bit of chaos.

Since my project isn't just an example, what ideas do you have to handle this?

@Aulddays

This comment has been minimized.

Show comment
Hide comment
@Aulddays

Aulddays Apr 18, 2017

Contributor

@edkimmel did you finally get it solved? The reference number / status code way as mentioned previously is still the best I can think of by now.

Contributor

Aulddays commented Apr 18, 2017

@edkimmel did you finally get it solved? The reference number / status code way as mentioned previously is still the best I can think of by now.

@edkimmel

This comment has been minimized.

Show comment
Hide comment
@edkimmel

edkimmel Apr 19, 2017

I've only been able to reproduce the socket being closed and reused immediately when CURLMOPT_PIPELINING is set to CURLPIPE_MULTIPLEX . I wasn't able to solve it, so I just removed that flag for now. I got stuck at a crash in sock_cb because I had no way to tie that call to a specific ref number.

I haven't explored this option yet:

  • close_socket flags the socket as "to-be-closed" in socket_map.
  • The actual close/removal is posted to the io_service so that it is guaranteed to be after the pending actions.
  • As we use socket_map, early quit on sockets that are flagged "to-be-closed"

In theory, this solves the ordering problem. There might still be issues with remsock, but those can be worked out.

edkimmel commented Apr 19, 2017

I've only been able to reproduce the socket being closed and reused immediately when CURLMOPT_PIPELINING is set to CURLPIPE_MULTIPLEX . I wasn't able to solve it, so I just removed that flag for now. I got stuck at a crash in sock_cb because I had no way to tie that call to a specific ref number.

I haven't explored this option yet:

  • close_socket flags the socket as "to-be-closed" in socket_map.
  • The actual close/removal is posted to the io_service so that it is guaranteed to be after the pending actions.
  • As we use socket_map, early quit on sockets that are flagged "to-be-closed"

In theory, this solves the ordering problem. There might still be issues with remsock, but those can be worked out.

@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.