Skip to content

Commit 77ba049

Browse files
committed
libstore: fix data race in getFileTransfer() singleton replacement
Multiple threads could simultaneously observe that the singleton FileTransfer instance has quit and attempt to replace it without synchronization. This caused undefined behavior as destroying the old object while other threads might still be accessing its mutex is not allowed. Fixed by implementing in-place restart of the FileTransfer object instead of replacing it. This avoids destroying mutexes that other threads might be waiting on or holding.
1 parent fca6d8f commit 77ba049

File tree

1 file changed

+47
-6
lines changed

1 file changed

+47
-6
lines changed

src/libstore/filetransfer.cc

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -611,11 +611,10 @@ struct curlFileTransfer : public FileTransfer
611611

612612
std::thread workerThread;
613613

614-
curlFileTransfer()
615-
: mt19937(rd())
614+
void resetCurl()
616615
{
617-
static std::once_flag globalInit;
618-
std::call_once(globalInit, curl_global_init, CURL_GLOBAL_ALL);
616+
if (curlm)
617+
curl_multi_cleanup(curlm);
619618

620619
curlm = curl_multi_init();
621620

@@ -625,13 +624,27 @@ struct curlFileTransfer : public FileTransfer
625624
#if LIBCURL_VERSION_NUM >= 0x071e00 // Max connections requires >= 7.30.0
626625
curl_multi_setopt(curlm, CURLMOPT_MAX_TOTAL_CONNECTIONS, fileTransferSettings.httpConnections.get());
627626
#endif
627+
}
628+
629+
void startWorkerThread()
630+
{
631+
workerThread = std::thread([&]() { workerThreadEntry(); });
632+
}
633+
634+
curlFileTransfer()
635+
: mt19937(rd())
636+
{
637+
static std::once_flag globalInit;
638+
std::call_once(globalInit, curl_global_init, CURL_GLOBAL_ALL);
639+
640+
resetCurl();
628641

629642
#ifndef _WIN32 // TODO need graceful async exit support on Windows?
630643
wakeupPipe.create();
631644
fcntl(wakeupPipe.readSide.get(), F_SETFL, O_NONBLOCK);
632645
#endif
633646

634-
workerThread = std::thread([&]() { workerThreadEntry(); });
647+
startWorkerThread();
635648
}
636649

637650
~curlFileTransfer()
@@ -644,6 +657,34 @@ struct curlFileTransfer : public FileTransfer
644657
curl_multi_cleanup(curlm);
645658
}
646659

660+
std::mutex restartMutex;
661+
662+
void restart()
663+
{
664+
std::lock_guard<std::mutex> restartLock(restartMutex);
665+
666+
// Check if we need to restart
667+
{
668+
auto state(state_.lock());
669+
if (!state->quit) {
670+
return;
671+
}
672+
}
673+
674+
// The worker thread will exit if quit has been set
675+
workerThread.join();
676+
677+
resetCurl();
678+
679+
// Reset the quit flag BEFORE starting the new thread
680+
{
681+
auto state(state_.lock());
682+
state->quit = false;
683+
}
684+
685+
startWorkerThread();
686+
}
687+
647688
void stopWorkerThread()
648689
{
649690
/* Signal the worker thread to exit. */
@@ -846,7 +887,7 @@ ref<FileTransfer> getFileTransfer()
846887
static ref<curlFileTransfer> fileTransfer = makeCurlFileTransfer();
847888

848889
if (fileTransfer->state_.lock()->quit)
849-
fileTransfer = makeCurlFileTransfer();
890+
fileTransfer->restart();
850891

851892
return fileTransfer;
852893
}

0 commit comments

Comments
 (0)