View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0004889 | libmicrohttpd | thread pool | public | 2017-02-10 13:59 | 2017-04-11 22:00 |
| Reporter | tomasheran | Assigned To | Christian Grothoff | Due Date | |
| Priority | normal | Severity | crash | Reproducibility | random |
| Status | closed | Resolution | fixed | ||
| Platform | Any | OS | Solaris | OS Version | 11.next |
| Product Version | Git master | ||||
| Target Version | 0.9.53 | Fixed in Version | 0.9.53 | ||
| Summary | 0004889: Race condition between MHD_cleanup_connections() and MHD_upgrade_action() | ||||
| Description | When MHD_USE_THREAD_PER_CONNECTION option is set, MHD_upgrade_action() will eventually shutdown() given connection's socket_fd and assign MHD_INVALID_SOCKET to it. When at the same time some other thread runs MHD_cleanup_connections() where there's this bit of a code: 2629 if (MHD_INVALID_SOCKET != pos->socket_fd) 2630 MHD_socket_close_chk_ (pos->socket_fd); I.e. it first checks whether the pos->socket_fd is not MHD_INVALID_SOCKET and if not, it goes on to close it (and check that the close succeeded or at least didn't file with EBADF). Because of the race-condition, pos->socket_fd can change from something valid to MHD_INVALID_SOCKET "between" lines 2629 and 2630, which will of course wreak havoc late (MHD_PANIC kind of havoc ;)). | ||||
| Steps To Reproduce | test_upgrade test seems to catch this fairly often on a slower (or busy) machine. | ||||
| Additional Information | Locking the daemon->cleanup_connection_mutex in MHD_upgrade_action() fixes the issue for me but I am not the right person to decide whether that's not too coarse grained lock to take at that time. | ||||
| Tags | No tags attached. | ||||
| This has already been fixed in Git in October: https://gnunet.org/git/libmicrohttpd.git/commit/?id=7d0221189929a3251d956166f82b06c456b33ed9 | |
| The race I see is between MHD_cleanup_connections() in daemon.c and MHD_upgrade_action() in response.c line 683. | |
| Yes, which is the one that the commit I quote resolves by _just_ calling shutdown and not closing the socket. That said, there was a 2nd issue in this constellation which I just fixed in 8a84902f7447edd6f677dfa9983c16146dacc7c5 Also, if you use line numbers, please state exactly which version (or Git revision) you are referencing, otherwise it's a bit hard to figure out what you are talking about. | |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2017-02-10 13:59 | tomasheran | New Issue | |
| 2017-02-12 08:49 | Christian Grothoff | Status | new => acknowledged |
| 2017-02-14 15:35 | Christian Grothoff | Note Added: 0011751 | |
| 2017-02-14 15:35 | Christian Grothoff | Assigned To | => Christian Grothoff |
| 2017-02-14 15:35 | Christian Grothoff | Status | acknowledged => resolved |
| 2017-02-14 15:35 | Christian Grothoff | Resolution | open => fixed |
| 2017-02-14 15:35 | Christian Grothoff | Fixed in Version | => 0.9.53 |
| 2017-02-14 15:35 | Christian Grothoff | Product Version | => Git master |
| 2017-02-14 15:35 | Christian Grothoff | Target Version | => 0.9.53 |
| 2017-02-14 16:05 | tomasheran | Status | resolved => feedback |
| 2017-02-14 16:05 | tomasheran | Resolution | fixed => reopened |
| 2017-02-14 16:05 | tomasheran | Note Added: 0011752 | |
| 2017-02-14 16:11 | Christian Grothoff | Note Added: 0011753 | |
| 2017-02-14 16:11 | Christian Grothoff | Status | feedback => resolved |
| 2017-02-14 16:11 | Christian Grothoff | Resolution | reopened => fixed |
| 2017-04-11 22:00 | Christian Grothoff | Status | resolved => closed |
| 2024-01-21 12:25 | Christian Grothoff | Category | libmicrohttpd multi-threaded operation => thread pool |