On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares <matheus.bernardino@xxxxxx> wrote: > > Some thread-unsafe functions of our codebase appear very down in the > call stack, which can be hard to notice (or avoid). Thus they are > sometimes used in threaded code unsafely. In this series we add > pthread_once() to compat/win32/ and use it in conjunction with > pthread_key to make a subset of the said functions thread-safe. Great! > As a next step, I would love to make [warning|error|die]_errno() > thread-safe as well. strerror() is not safe on *nix, and there are some > thread functions today that call these (although the actual risk of a > race condition must be very small...) > > My idea was to implement a xstrerror() wrapper which calls the > appropriate thread-safe function (dependant on the OS), Yeah, that works if there are appropriate thread-safe functions for all the OS we are interested in, or if we can fallback to strerror() or calling it holding a lock. > or even call > strerror() itself but holding a lock to copy the result for a local > buffer (which should be OK as we don't expect contention in strerror). I agree that it should be ok. > We could also set a thread local buffer array, as in the second patch of > this series, to excuse callers from allocating/freeing memory. I don't think caller allocating/freeing memory for error strings is a performance or code simplification issue. > One concern with this idea is the risk of an infinite recursion if > xstrerror() or any of its childs call [warning|error|die]_errno(). > However, if we are only using strerror() and pthread_*() within the > wrapper, there should be no problem, right? Yeah, I agree. > Has anyone thought of > other problems with this approach? > > Finally, should such change also come with a coccinelle patch to replace > usages of strerror() with xstrerror()? Or better not, as the change > would be too big? I would agree that it's better to avoid too big changes. I am not sure how much we want to automate and check that though. Thanks, Christian.
