From 05d749baaeb75643882b16f0c864c0f8f3eb7308 Mon Sep 17 00:00:00 2001 From: Stephen Morris Date: Wed, 12 Jun 2019 14:35:07 +0100 Subject: [PATCH] [#640,!351] Fixed problem in synchronization between threads. --- src/lib/dhcpsrv/fuzz.cc | 127 ++++++++++++++++++++++-------- src/lib/dhcpsrv/fuzz.h | 48 +++++------ src/lib/dhcpsrv/fuzz_messages.mes | 18 ++++- 3 files changed, 130 insertions(+), 63 deletions(-) diff --git a/src/lib/dhcpsrv/fuzz.cc b/src/lib/dhcpsrv/fuzz.cc index 0824355c69..62ff051cd7 100644 --- a/src/lib/dhcpsrv/fuzz.cc +++ b/src/lib/dhcpsrv/fuzz.cc @@ -38,9 +38,11 @@ constexpr useconds_t Fuzz::SLEEP_INTERVAL; constexpr long Fuzz::LOOP_COUNT; // Variables needed to synchronize between main and fuzzing threads. -condition_variable Fuzz::sync_cond_; -mutex Fuzz::sync_mutex_; -thread Fuzz::fuzz_thread_; +condition_variable Fuzz::fuzz_cond_; +mutex Fuzz::fuzz_mutex_; +thread Fuzz::fuzzing_thread_; +condition_variable Fuzz::main_cond_; +mutex Fuzz::main_mutex_; // Address structure used to send data to address/port on which Kea listens struct sockaddr_in6 Fuzz::servaddr_; @@ -49,6 +51,10 @@ struct sockaddr_in6 Fuzz::servaddr_; // after the appropriate number of packets have been fuzzed. volatile bool* Fuzz::shutdown_ptr_ = NULL; +// Variable used in condition checks +volatile bool Fuzz::fuzz_ready_ = false; +volatile bool Fuzz::main_ready_ = false; + // Flag to state that the @@ -113,13 +119,25 @@ Fuzz::init(volatile bool* shutdown_flag) { servaddr_.sin6_port = htons(port); servaddr_.sin6_scope_id = iface_id; + // Ensure that condition variables are initially clear. + fuzz_ready_ = false; + main_ready_ = false; + // Initialization complete. LOG_INFO(fuzz_logger, FUZZ_INTERFACE) .arg(iface_ptr).arg(address_ptr).arg(port); // Start the thread that reads the packets sent by AFL from stdin and - // passes them to the port on which Kea is listening. - fuzz_thread_ = std::thread(Fuzz::main); + // passes them to the port on which Kea is listening. We'll ensure + // the the fuzzing thread has read its first packet from the fuzzer + // before continuing. + unique_lock lock(fuzz_mutex_); + fuzzing_thread_ = std::thread(Fuzz::run); + LOG_DEBUG(fuzz_logger, 1, FUZZ_WAI).arg("fuzz_cond").arg("init"); + fuzz_cond_.wait(lock, []{return fuzz_ready_;}); + fuzz_ready_ = false; + LOG_DEBUG(fuzz_logger, 1, FUZZ_CWT).arg("fuzz_cond").arg("init"); + lock.unlock(); } catch (const FuzzInitFail& e) { // AFL tends to make it difficult to find out what exactly has failed: @@ -134,7 +152,7 @@ Fuzz::init(volatile bool* shutdown_flag) { // Then it wait for a conditional, which is called in kea_fuzz_notify() from // Kea main loop. void -Fuzz::main(void) { +Fuzz::run(void) { // Create the socket throw which packets read from stdin will be send // to the port on which Kea is listening. int sockfd = socket(AF_INET6, SOCK_DGRAM, 0); @@ -151,6 +169,11 @@ Fuzz::main(void) { // of the fuzzing process. auto loop = Fuzz::LOOP_COUNT; while (loop-- > 0) { + // Prevent the main thread from reading until we have made a packet + // available. Although the read in Kea itself is not protected by + // a mutex, the condition variable used to control when the main + // thread runs is. + unique_lock fuzz_lock(fuzz_mutex_); // Read from stdin and continue reading (albeit after a pause) even // if there is an error. Do the same with end of files although, as @@ -181,7 +204,6 @@ Fuzz::main(void) { // code doesn't work. // Block the completion function until the data has been sent: - unique_lock lock(sync_mutex_); // Send the data to the main Kea thread. ssize_t sent = sendto(sockfd, buf, length, 0, @@ -194,40 +216,58 @@ Fuzz::main(void) { LOG_DEBUG(fuzz_logger, FUZZ_DBG_TRACE_DETAIL, FUZZ_SEND).arg(sent); } - // If this is the last loop iteration, set the shutdown flag. This - // is done under the protection of the mutex to avoid the following - // scenario: - // - // a) This loop reaches its last iteration, then waits for - // notify() to complete. - // b) notify() completes and returns. The main processing loop checks - // the shutdown flag and finds that it is not set. It enters the - // next iteration of the loop and waits to read something from the - // configured interface. - // c) This thread resumes execution, sets the shutdown flag and exits. - // - // This would leave the main processing loop waiting for a packet that - // will never arrive. - // - // By setting the shutdown flag under the protection of the mutex, - // the notify() call will not take place until the flag is set, and - // the main thread will see the flag being set and exit. We might - // as well close the socket at the same time. if (loop <= 0) { + // If this is the last loop iteration, set the shutdown flag. This + // is done under the protection of the mutex to avoid the following + // scenario: + // + // a) This loop reaches its last iteration, then waits for + // notify() to complete. + // b) notify() completes and returns. The main processing loop + // check the shutdown flag and finds that it is not set. It + // enters the next iteration of the loop and waits to read + // something from the configured interface. + // c) This thread resumes execution, sets the shutdown flag and + // exits. + // + // This would leave the main processing loop waiting for a packet + // that will never arrive. + // + // By setting the shutdown flag under the protection of the mutex, + // the notify() call will not take place until the flag is set, and + // the main thread will see the flag being set and exit. We might + // as well close the socket at the same time. *shutdown_ptr_ = true; close(sockfd); + LOG_DEBUG(fuzz_logger, 1, FUZZ_SHUTDOWN_INITIATED); } + LOG_DEBUG(fuzz_logger, 1, FUZZ_SET).arg("fuzz_cond-1").arg("main"); + fuzz_ready_ = true; + fuzz_cond_.notify_all(); + LOG_DEBUG(fuzz_logger, 1, FUZZ_CST).arg("fumz_cond-1").arg("main"); + fuzz_lock.unlock(); // We now need to synchronize with the main thread. In particular, // we suspend processing until we know that the processing of the // packet by Kea has finished and that the completion function has // raised a SIGSTOP. - sync_cond_.wait(lock); - lock.unlock(); + unique_lock main_lock(main_mutex_); + LOG_DEBUG(fuzz_logger, 1, FUZZ_WAI).arg("main_cond").arg("main"); + main_cond_.wait(main_lock, [] {return main_ready_;}); + LOG_DEBUG(fuzz_logger, 1, FUZZ_CWT).arg("main_cond").arg("main"); + main_ready_ = false; + main_lock.unlock(); } - // Loop has exited, so we should shut down Kea. Tidy up and signal Kea - // to exit. + // If the main thread is waiting, let it terminate as well. + unique_lock fuzz_lock(fuzz_mutex_); + fuzz_ready_ = true; + LOG_DEBUG(fuzz_logger, 1, FUZZ_SET).arg("fuzz_cond-2").arg("main"); + fuzz_cond_.notify_all(); + LOG_DEBUG(fuzz_logger, 1, FUZZ_CST).arg("fuzz_cond-2").arg("main"); + fuzz_lock.unlock(); + + // Loop has exited, so we should shut down Kea. LOG_DEBUG(fuzz_logger, FUZZ_DBG_TRACE, FUZZ_LOOP_EXIT); return; @@ -236,7 +276,14 @@ Fuzz::main(void) { // Waits for the fuzzing thread to terminate. void Fuzz::wait(void) { - fuzz_thread_.join(); + unique_lock main_lock(main_mutex_); + main_ready_ = true; + LOG_DEBUG(fuzz_logger, 1, FUZZ_SET).arg("main_cond").arg("wait"); + main_cond_.notify_all(); + LOG_DEBUG(fuzz_logger, 1, FUZZ_CST).arg("main_cond").arg("wait"); + main_lock.unlock(); + LOG_DEBUG(fuzz_logger, 1, FUZZ_THREAD_WAIT); + fuzzing_thread_.join(); } // Called by the main thread, this notifies AFL that processing for the @@ -245,8 +292,22 @@ void Fuzz::notify(void) { LOG_DEBUG(fuzz_logger, FUZZ_DBG_TRACE_DETAIL, FUZZ_NOTIFY_CALLED); raise(SIGSTOP); - lock_guard lock(sync_mutex_); - sync_cond_.notify_all(); + + // Tell the fuzzing loop that it can continue. + unique_lock main_lock(main_mutex_); + main_ready_ = true; + LOG_DEBUG(fuzz_logger, 1, FUZZ_SET).arg("main_cond").arg("notify"); + main_cond_.notify_all(); + LOG_DEBUG(fuzz_logger, 1, FUZZ_CST).arg("main_cond").arg("notify"); + main_lock.unlock(); + + // ... and wait until it tells us that it can continue. + unique_lock fuzz_lock(fuzz_mutex_); + LOG_DEBUG(fuzz_logger, 1, FUZZ_WAI).arg("fuzz_cond").arg("notify"); + fuzz_cond_.wait(fuzz_lock, []{return fuzz_ready_;}); + LOG_DEBUG(fuzz_logger, 1, FUZZ_CWT).arg("fuzz_cond").arg("notify"); + fuzz_ready_ = false; + fuzz_lock.unlock(); } #endif // ENABLE_AFL diff --git a/src/lib/dhcpsrv/fuzz.h b/src/lib/dhcpsrv/fuzz.h index 341ff42561..f585b90f8a 100644 --- a/src/lib/dhcpsrv/fuzz.h +++ b/src/lib/dhcpsrv/fuzz.h @@ -55,19 +55,7 @@ public: /// logger. (Other than initialization - when the thread is not running - /// this is the only use of the fuzzing logger.) If the error is fatal, the /// thread will terminate, something that may cause the fuzzer to hang. - static void kea_fuzz_main(void); - - /// @brief Fuzzing Thread - /// - /// This function is run in a separate thread. It loops, reading input - /// from AFL that appears on stdin and writing it to the address/port on - /// which Kea is listening. - /// - /// After writing the data to the Kea address/port, the function waits - /// until processing of the data is complete (as indicated by a condition - /// variable set by the notification function) before reading the next - /// input. - static void main(void); + static void run(void); /// @brief Notify fuzzing thread that processing is complete /// @@ -98,22 +86,24 @@ public: /// processing packets properly. static constexpr long LOOP_COUNT = 1000; - // Member variables - - /// @brief Condition variable to synchronize between threads. - static std::condition_variable sync_cond_; - - /// @brief Mutex to synchronize between threads. - static std::mutex sync_mutex_; - - /// @brief Holds the separate thread that reads from stdin - static std::thread fuzz_thread_; - - /// @brief Socket structure used for sending data to main thread - static struct sockaddr_in6 servaddr_; - - /// @brief Pointer to the Kea shutdown flag - static volatile bool* shutdown_ptr_; + // Condition/mutext variables. The fuzz_XX_ variables are set by the + // fuzzing thread and waited on by the main thread. The main_XX_ variables + // are set by the main thread and waited on by the fuzzing thread. + static std::condition_variable fuzz_cond_; //< Set by fuzzing thread + static std::mutex fuzz_mutex_; //< Set by fuzzing thread + static std::condition_variable main_cond_; //< Set by main thread + static std::mutex main_mutex_; //< Set by main thread + + // The next two variables are used in the condition variables test. fuzz_ready_ + // i set by the fuzzing thread and cleared by the main thread. main_ready_ + // is set by the main thread and cleared by the fuzzing thread. + static volatile bool fuzz_ready_; //< Set when data has been read from fuzzer + static volatile bool main_ready_; //< Set when data has been read from fuzzer + + // Other member variables. + static std::thread fuzzing_thread_;//< Holds the thread ID + static struct sockaddr_in6 servaddr_; //< For sending data to main thread + static volatile bool* shutdown_ptr_; //< Pointer to shutdown flag }; diff --git a/src/lib/dhcpsrv/fuzz_messages.mes b/src/lib/dhcpsrv/fuzz_messages.mes index 86ec49945d..355144ffb5 100644 --- a/src/lib/dhcpsrv/fuzz_messages.mes +++ b/src/lib/dhcpsrv/fuzz_messages.mes @@ -54,4 +54,20 @@ communications between the fuzzing thread and the main Kea processing; the c % FUZZ_SOCKET_CREATE_FAIL failed to crease socket for use by fuzzing thread: %1 An error message output when the fuzzing code has failed to create a socket through which is will copy data received on stdin from the AFL fuzzer to -the port on which +the port on which Kea is listening. + +% FUZZ_WAI %2: %1 +A debug message stating what is being waited for. + +% FUZZ_SET %2: %1 +A debug message stating what is being set. + +% FUZZ_CWT %2: %1 + +% FUZZ_CST %2: %1 + +% FUZZ_THREAD_WAIT waiting for thread to die + +% FUZZ_SHUTDOWN_INITIATED shutdown initated, shutdown flag is set + +% FUZZ_GENERAL %1 -- 2.47.2