AprilRobotics / apriltag

AprilTag is a visual fiducial system popular for robotics research.
https://april.eecs.umich.edu/software/apriltag
Other
1.47k stars 522 forks source link

Fix spurious wake-ups and worker threads initialization in workerpool #339

Closed berteauxjb closed 1 week ago

berteauxjb commented 1 week ago

I encountered random segfaults while using this library and tried to figure out what was happening. I isolated the segfault to the function worker_thread() of workerpool.c. Long story short, there is no predicate that prevents worker_thread() from running if it is woken up spuriously and a task has already been added. Since there is no mutex used in workerpool_add_task(), it is possible for the zarray wp->tasks to be reallocated, rendering the volatile pointer task in worker_thread() invalid and causing a segfault.

This PR addresses this issue by adding a predicate and shielding calls that access wp behind a mutex. I also saw issue #70 and agree that the initialization of the worker threads might lead to race conditions, so I addressed that too.

christian-rauch commented 1 week ago

Thanks for the PR. I approved the CI for this PR. The tests crash on Windows (Exception: SegFault). Any idea what is going on there?

berteauxjb commented 1 week ago

Thanks for the PR. I approved the CI for this PR. The tests crash on Windows (Exception: SegFault). Any idea what is going on there?

I was able to reproduce on a Windows machine :smile: It turns out that the wp->mutex is only initialized if wp->nthreads > 1. In the test there is only 1 thread, and workerpool_add_task was accessing a non-initialized mutex. I shielded the usage of the mutex with if (wp->nthreads > 1) like it's done in other places in the file.

berteauxjb commented 1 week ago

Final request: Can you now squash your fixup commit 737ff6b into the original commit be17a2d and force-push again with the clean git history?

Done!

christian-rauch commented 1 week ago

Thanks a lot!