{"thread":{"id":"53752","subject":"[RFC] Thread safety in some low-level functions","startedAt":"2020-06-24T22:29:48Z","lastAt":"2020-08-10T14:15:35Z","messageCount":25,"participants":["Matheus Tavares Bernardino","brian m. carlson","Matheus Tavares","Christian Couder","Johannes Schindelin","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"400560","messageId":"CAHd-oW5zh=BG29O0Z-M7R26Lgd=RHECMV2+qByF+vU6PmrEn_Q@mail.gmail.com","threadId":"53752","inReplyTo":null,"subject":"[RFC] Thread safety in some low-level functions","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-24T22:29:32Z","receivedAt":"2020-06-24T22:29:48Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi,\n\nI'm working with threads in unpack-trees and noticed that\n[warning|error]_errno() uses strerror(), which is not thread-safe. We\ncould try to avoid calling these functions in threaded code, but they\nare sometimes too deep in the call stack to be noticed... (or even\navoided). The same happens with oid_to_hex(), which writes to a static\nbuffer.\n\nI don't think I've ever seen a bug report involving these functions\nbeing called racily, but this possibility is not currently excluded in\nour codebase. For example, see grep_source_load_file(), which is\ncalled by multiple threads concurrently and might call the\nthread-unsafe error_errno(). (Although, I admit, the chance of a race\nhere must be very low...)\n\nI still haven't been able to come up with a simple / easy change that\ncould make these functions thread safe, but here are my thoughts so\nfar:\n\n- For strerror(), there is a thread-safe variant: strerror_r().\nHowever IIUC, this variant is not present on Windows (although there\nis strerror_s() which *seems* to be somewhat similar). Also, there are\ntwo versions of strerror_r() on Linux: one is XSI-compliant and the\nother is GNU-specific. I don't know what the situation is in other\nOSes...\n\n- Regarding, oid_to_hex(), a patch from 2010 [1] proposed a solution\nusing thread-local storage and pthread_once(). But as Hannes pointed\nout in this other thread [2] , implementing a Windows equivalence for\npthread_once() could be tricky and voluminous. Since this thread dates\nfrom 7 years ago, I was wondering if we would be able to implement it\nnowadays with InitOnceExecuteOnce() [3].\n\nFinally, leaving these functions thread-unsafe is also a\npossibility... As I mentioned earlier, they don't seem to be causing\nproblems out there for now (at least not reported). But if we can find\na feasible solution to introduce thread-safety, I think it would be\ngreat. The codebase would be more robust and we would be able to work\nwith threads with much more confidence.\n\nAny thoughts?\n\nThanks,\nMatheus\n\n[1] https://lore.kernel.org/git/20100323173130.GC4218@fredrik-laptop/\n[2] https://lore.kernel.org/git/516D5CA4.7000500@viscovery.net/\n[3]: https://docs.microsoft.com/en-us/windows/win32/sync/using-one-time-initialization\n"},{"id":"400562","messageId":"CAHd-oW5jCWeWDcMhLU2Zf96uTtxLehVsM+StDC8LZga282cDjw@mail.gmail.com","threadId":"53752","inReplyTo":"CAHd-oW5zh=BG29O0Z-M7R26Lgd=RHECMV2+qByF+vU6PmrEn_Q@mail.gmail.com","subject":"Re: [RFC] Thread safety in some low-level functions","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-24T22:52:58Z","receivedAt":"2020-06-24T22:53:14Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Jun 24, 2020 at 7:29 PM Matheus Tavares Bernardino\n wrote:\n>\n> - Regarding, oid_to_hex(), a patch from 2010 [1] proposed a solution\n> using thread-local storage and pthread_once(). But as Hannes pointed\n> out in this other thread [2] , implementing a Windows equivalence for\n> pthread_once() could be tricky and voluminous. Since this thread dates\n> from 7 years ago, I was wondering if we would be able to implement it\n> nowadays with InitOnceExecuteOnce() [3].\n\nI forgot to link it before, but here is a commit in libav.git which\nemulates pthread_once():\nhttps://git.libav.org/?p=libav.git;a=commitdiff;h=b22693b06d1e5d73454a65c203b4d31c1ca5b69a\n\nThe patch is not so big, and if we consider only Vista+, it seems very\nstraightforward. I'm not very familiar with Windows, though, and I'm\nnot sure if this solution would work for us.\n"},{"id":"400569","messageId":"20200625013851.GA9782@camp.crustytoothpaste.net","threadId":"53752","inReplyTo":"CAHd-oW5jCWeWDcMhLU2Zf96uTtxLehVsM+StDC8LZga282cDjw@mail.gmail.com","subject":"Re: [RFC] Thread safety in some low-level functions","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-06-25T01:38:51Z","receivedAt":"2020-06-25T01:39:31Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-06-24 at 22:52:58, Matheus Tavares Bernardino wrote:\n> On Wed, Jun 24, 2020 at 7:29 PM Matheus Tavares Bernardino\n> wrote:\n> >\n> > - Regarding, oid_to_hex(), a patch from 2010 [1] proposed a solution\n> > using thread-local storage and pthread_once(). But as Hannes pointed\n> > out in this other thread [2] , implementing a Windows equivalence for\n> > pthread_once() could be tricky and voluminous. Since this thread dates\n> > from 7 years ago, I was wondering if we would be able to implement it\n> > nowadays with InitOnceExecuteOnce() [3].\n> \n> I forgot to link it before, but here is a commit in libav.git which\n> emulates pthread_once():\n> https://git.libav.org/?p=libav.git;a=commitdiff;h=b22693b06d1e5d73454a65c203b4d31c1ca5b69a\n> \n> The patch is not so big, and if we consider only Vista+, it seems very\n> straightforward. I'm not very familiar with Windows, though, and I'm\n> not sure if this solution would work for us.\n\nI believe Git for Windows dropped support for Windows XP some time back.\nRegardless, since pretty much nobody using Windows XP (with the possible\nexception of ATMs) is getting security support, I'm fine with ignoring\nit either way. I am also not a Windows person, so others may have more\ninsight.\n\nAs for the general idea, I'm mildly in favor of it. While I don't plan\nto work on the project myself (outside of cleaning up places I'm already\nworking), I'm definitely not opposed to seeing patches come in to\nimprove thread safety.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"400640","messageId":"cover.1593115455.git.matheus.bernardino@usp.br","threadId":"53752","inReplyTo":"20200625013851.GA9782@camp.crustytoothpaste.net","subject":"[PATCH 0/2] Make oid_to_hex() thread-safe","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-25T20:32:55Z","receivedAt":"2020-06-25T20:33:09Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Some thread-unsafe functions of our codebase appear very down in the\ncall stack, which can be hard to notice (or avoid). Thus they are\nsometimes used in threaded code unsafely. In this series we add\npthread_once() to compat/win32/ and use it in conjunction with\npthread_key to make a subset of the said functions thread-safe.\n\nAs a next step, I would love to make [warning|error|die]_errno()\nthread-safe as well. strerror() is not safe on *nix, and there are some\nthread functions today that call these (although the actual risk of a\nrace condition must be very small...)\n\nMy idea was to implement a xstrerror() wrapper which calls the\nappropriate thread-safe function (dependant on the OS), or even call\nstrerror() itself but holding a lock to copy the result for a local\nbuffer (which should be OK as we don't expect contention in strerror).\nWe could also set a thread local buffer array, as in the second patch of\nthis series, to excuse callers from allocating/freeing memory.\n\nOne concern with this idea is the risk of an infinite recursion if\nxstrerror() or any of its childs call [warning|error|die]_errno().\nHowever, if we are only using strerror() and pthread_*() within the\nwrapper, there should be no problem, right? Has anyone thought of\nother problems with this approach?\n\nFinally, should such change also come with a coccinelle patch to replace\nusages of strerror() with xstrerror()? Or better not, as the change\nwould be too big?\n\nMatheus Tavares (2):\n compat/win32/pthread: add pthread_once()\n hex: make hash_to_hex_algop() and friends thread-safe\n\n compat/win32/pthread.c | 22 +++++++++++++++++++++\n compat/win32/pthread.h | 5 +++++\n hex.c | 45 ++++++++++++++++++++++++++++++++++++++----\n thread-utils.c | 11 +++++++++++\n thread-utils.h | 6 ++++++\n 5 files changed, 85 insertions(+), 4 deletions(-)\n\n-- \n2.26.2\n\n"},{"id":"400641","messageId":"e5a10d3f2152859b75bd815c37511975057d0ab0.1593115455.git.matheus.bernardino@usp.br","threadId":"53752","inReplyTo":"cover.1593115455.git.matheus.bernardino@usp.br","subject":"[PATCH 1/2] compat/win32/pthread: add pthread_once()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-25T20:32:56Z","receivedAt":"2020-06-25T20:33:12Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Signed-off-by: Matheus Tavares \n---\n\nNote: the pthread_once() function is adapted from:\nhttps://git.libav.org/?p=libav.git;a=commitdiff;h=b22693b06d1e5d73454a65c203b4d31c1ca5b69a\n\nWhich is LGPLv2.1. Should I add any notice/acknowledgment somewhere,\nbesides the comment I added above the function?\n\n compat/win32/pthread.c | 22 ++++++++++++++++++++++\n compat/win32/pthread.h | 5 +++++\n thread-utils.c | 11 +++++++++++\n thread-utils.h | 6 ++++++\n 4 files changed, 44 insertions(+)\n\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nindex 2e7eead42c..5a7ecbd999 100644\n--- a/compat/win32/pthread.c\n+++ b/compat/win32/pthread.c\n@@ -56,3 +56,25 @@ pthread_t pthread_self(void)\n \tt.tid = GetCurrentThreadId();\n \treturn t;\n }\n+\n+/* Adapted from libav's compat/w32pthreads.h. */\n+int pthread_once(pthread_once_t *once_control, void (*init_routine)(void))\n+{\n+\tBOOL pending = FALSE;\n+\tint ret = 0;\n+\n+\tif(!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {\n+\t\tret = err_win_to_posix(GetLastError());\n+\t\tgoto out;\n+\t}\n+\n+\tif (pending)\n+\t\tinit_routine();\n+\n+\tif(!InitOnceComplete(once_control, 0, NULL))\n+\t\tret = err_win_to_posix(GetLastError());\n+\n+out:\n+\t/* POSIX doesn't allow pthread_once() to return EINTR */\n+\treturn ret == EINTR ? EIO : ret;\n+}\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nindex 737983d00b..c50f1e89c7 100644\n--- a/compat/win32/pthread.h\n+++ b/compat/win32/pthread.h\n@@ -40,6 +40,11 @@ typedef int pthread_mutexattr_t;\n #define pthread_cond_signal WakeConditionVariable\n #define pthread_cond_broadcast WakeAllConditionVariable\n \n+#define pthread_once_t INIT_ONCE\n+\n+#define PTHREAD_ONCE_INIT INIT_ONCE_STATIC_INIT\n+int pthread_once(pthread_once_t *once_control, void (*init_routine)(void));\n+\n /*\n * Simple thread creation implementation using pthread API\n */\ndiff --git a/thread-utils.c b/thread-utils.c\nindex 5329845691..937deb3f2e 100644\n--- a/thread-utils.c\n+++ b/thread-utils.c\n@@ -122,4 +122,15 @@ int dummy_pthread_join(pthread_t pthread, void **retval)\n \treturn ENOSYS;\n }\n \n+int nothreads_pthread_once(pthread_once_t *once_control,\n+\t\t\t void (*init_routine)(void))\n+{\n+\tif (*once_control == 1)\n+\t\treturn 0;\n+\n+\tinit_routine();\n+\t*once_control = 1;\n+\treturn 0;\n+}\n+\n #endif\ndiff --git a/thread-utils.h b/thread-utils.h\nindex 4961487ed9..bab9dc5e4d 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -19,6 +19,7 @@\n #define pthread_mutex_t int\n #define pthread_cond_t int\n #define pthread_key_t int\n+#define pthread_once_t int\n \n #define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)\n #define pthread_mutex_lock(mutex)\n@@ -48,6 +49,11 @@ int dummy_pthread_join(pthread_t pthread, void **retval);\n \n int dummy_pthread_init(void *);\n \n+#define PTHREAD_ONCE_INIT 0\n+int nothreads_pthread_once(pthread_once_t *once_control,\n+\t\t\t void (*init_routine)(void));\n+#define pthread_once(once, routine) nothreads_pthread_once(once, routine)\n+\n #endif\n \n int online_cpus(void);\n-- \n2.26.2\n\n"},{"id":"400642","messageId":"0104cd9c763aee220a2df357834c79b10695ee35.1593115455.git.matheus.bernardino@usp.br","threadId":"53752","inReplyTo":"cover.1593115455.git.matheus.bernardino@usp.br","subject":"[PATCH 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-25T20:32:57Z","receivedAt":"2020-06-25T20:33:17Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"hash_to_hex_algop() returns a static buffer, relieving callers from the\nresponsibility of freeing memory after use. But the current\nimplementation uses the same static data for all threads and, thus, is\nnot thread-safe. We could avoid using this function and its wrappers\nin threaded code, but they are sometimes too deep in the call stack to\nbe noticed or even avoided.\n\nFor example, we can take a look at the number of oid_to_hex() calls,\nwhich calls hash_to_hex_algop():\n\n$ git grep 'oid_to_hex(' | wc -l\n818\n\nAlthough these functions don't seem to be causing problems out there for\nnow (at least not reported), making them thread-safe makes the codebase\nmore robust against race conditions. We can easily do that replicating\nthe static buffer in each thread's local storage.\n\nOriginal-patch-by: Fredrik Kuivinen \nSigned-off-by: Matheus Tavares \n---\n hex.c | 45 +++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 41 insertions(+), 4 deletions(-)\n\ndiff --git a/hex.c b/hex.c\nindex da51e64929..1094ed25bd 100644\n--- a/hex.c\n+++ b/hex.c\n@@ -136,12 +136,49 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)\n \treturn hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);\n }\n \n+struct hexbuf_array {\n+\tint idx;\n+\tchar bufs[4][GIT_MAX_HEXSZ + 1];\n+};\n+\n+#ifdef HAVE_THREADS\n+static pthread_key_t hexbuf_array_key;\n+static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT;\n+\n+void init_hexbuf_array_key(void)\n+{\n+\tif (pthread_key_create(&hexbuf_array_key, free))\n+\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n+}\n+\n+#else\n+static struct hexbuf_array default_hexbuf_array;\n+#endif\n+\n char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *algop)\n {\n-\tstatic int bufno;\n-\tstatic char hexbuffer[4][GIT_MAX_HEXSZ + 1];\n-\tbufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);\n-\treturn hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);\n+\tstruct hexbuf_array *ha;\n+\n+#ifdef HAVE_THREADS\n+\tvoid *value;\n+\n+\tif (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n+\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n+\n+\tvalue = pthread_key_getspecific(&hexbuf_array_key);\n+\tif (value) {\n+\t\tha = (struct hexbuf_array *) value;\n+\t} else {\n+\t\tha = xmalloc(sizeof(*ha));\n+\t\tif (pthread_key_setspecific(&hexbuf_array_key, (void *)ha))\n+\t\t\tdie(_(\"failed to set thread buffer for hash to hex conversion\"));\n+\t}\n+#else\n+\tha = &default_hexbuf_array;\n+#endif\n+\n+\tha->idx = (ha->idx + 1) % ARRAY_SIZE(ha->bufs);\n+\treturn hash_to_hex_algop_r(ha->bufs[ha->idx], hash, algop);\n }\n \n char *hash_to_hex(const unsigned char *hash)\n-- \n2.26.2\n\n"},{"id":"400647","messageId":"20200625230753.GB9782@camp.crustytoothpaste.net","threadId":"53752","inReplyTo":"0104cd9c763aee220a2df357834c79b10695ee35.1593115455.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-06-25T23:07:53Z","receivedAt":"2020-06-25T23:08:32Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-06-25 at 20:32:57, Matheus Tavares wrote:\n> +#ifdef HAVE_THREADS\n> +\tvoid *value;\n> +\n> +\tif (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n> +\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> +\n> +\tvalue = pthread_key_getspecific(&hexbuf_array_key);\n\nI think the portable name for this is \"pthread_getspecific\". That\nappears to be what POSIX specifies and what our Windows pthread compat\ncode uses.\n\n> +\tif (value) {\n> +\t\tha = (struct hexbuf_array *) value;\n> +\t} else {\n> +\t\tha = xmalloc(sizeof(*ha));\n> +\t\tif (pthread_key_setspecific(&hexbuf_array_key, (void *)ha))\n\nSame thing here.\n\n> +\t\t\tdie(_(\"failed to set thread buffer for hash to hex conversion\"));\n> +\t}\n> +#else\n> +\tha = &default_hexbuf_array;\n> +#endif\n> +\n> +\tha->idx = (ha->idx + 1) % ARRAY_SIZE(ha->bufs);\n> +\treturn hash_to_hex_algop_r(ha->bufs[ha->idx], hash, algop);\n> }\n> \n> char *hash_to_hex(const unsigned char *hash)\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"400648","messageId":"CAHd-oW7GzbtWONYiE9RHQRSzeB1gGvbojJEzdKSMg7qQTMCuOw@mail.gmail.com","threadId":"53752","inReplyTo":"20200625230753.GB9782@camp.crustytoothpaste.net","subject":"Re: [PATCH 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-25T23:54:43Z","receivedAt":"2020-06-25T23:55:00Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Jun 25, 2020 at 8:08 PM brian m. carlson\n wrote:\n>\n> On 2020-06-25 at 20:32:57, Matheus Tavares wrote:\n> > +#ifdef HAVE_THREADS\n> > + void *value;\n> > +\n> > + if (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n> > + die(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> > +\n> > + value = pthread_key_getspecific(&hexbuf_array_key);\n>\n> I think the portable name for this is \"pthread_getspecific\". That\n> appears to be what POSIX specifies and what our Windows pthread compat\n> code uses.\n\nRight, thanks for noticing that!\n\n(I wonder how the Windows build passed successfully in our CI [1]...\nShouldn't the compiler have failed due to a \"undefined reference\"\nerror?)\n\n[1]: https://github.com/matheustavares/git/runs/808649609\n"},{"id":"400649","messageId":"CAHd-oW4twBf-OskkJoNCxGO22mpWnVvjyiGT+MsJtF6W-J+Gyg@mail.gmail.com","threadId":"53752","inReplyTo":"CAHd-oW7GzbtWONYiE9RHQRSzeB1gGvbojJEzdKSMg7qQTMCuOw@mail.gmail.com","subject":"Re: [PATCH 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-26T00:00:41Z","receivedAt":"2020-06-26T00:00:56Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Jun 25, 2020 at 8:54 PM Matheus Tavares Bernardino\n wrote:\n>\n> On Thu, Jun 25, 2020 at 8:08 PM brian m. carlson\n> wrote:\n> >\n> > On 2020-06-25 at 20:32:57, Matheus Tavares wrote:\n> > > +#ifdef HAVE_THREADS\n> > > + void *value;\n> > > +\n> > > + if (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n> > > + die(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> > > +\n> > > + value = pthread_key_getspecific(&hexbuf_array_key);\n> >\n> > I think the portable name for this is \"pthread_getspecific\". That\n> > appears to be what POSIX specifies and what our Windows pthread compat\n> > code uses.\n>\n> Right, thanks for noticing that!\n>\n> (I wonder how the Windows build passed successfully in our CI [1]...\n> Shouldn't the compiler have failed due to a \"undefined reference\"\n> error?)\n\nOh, I know why... I forgot to include \"thread-utils.h\", so\nHAVE_THREADS is never defined and this code is always ignored (on both\nWindows and Linux). That's embarrassing... I'll correct that for v2.\n"},{"id":"400652","messageId":"CAP8UFD3NwKtWYL_H5fWjT-HQVd3W8ZJ49p+faDyMQ+==ttCWjA@mail.gmail.com","threadId":"53752","inReplyTo":"e5a10d3f2152859b75bd815c37511975057d0ab0.1593115455.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 1/2] compat/win32/pthread: add pthread_once()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-06-26T05:45:03Z","receivedAt":"2020-06-26T05:45:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 26, 2020 at 3:00 AM Matheus Tavares\n wrote:\n>\n> Signed-off-by: Matheus Tavares \n> ---\n\nThe commit message might want to explain a bit the purpose of adding\nthese features.\n\n> Note: the pthread_once() function is adapted from:\n> https://git.libav.org/?p=libav.git;a=commitdiff;h=b22693b06d1e5d73454a65c203b4d31c1ca5b69a\n>\n> Which is LGPLv2.1. Should I add any notice/acknowledgment somewhere,\n> besides the comment I added above the function?\n\nYeah, I think you should also tell in the commit message where the\ncode comes from (along with the hash of the commit) and that libav is\nLGPLv2.1 which is compatible with GPLv2 as explained in section 3 of\nthe LGPLv2.1.\n\n> compat/win32/pthread.c | 22 ++++++++++++++++++++++\n> compat/win32/pthread.h | 5 +++++\n> thread-utils.c | 11 +++++++++++\n> thread-utils.h | 6 ++++++\n> 4 files changed, 44 insertions(+)\n>\n> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\n> index 2e7eead42c..5a7ecbd999 100644\n> --- a/compat/win32/pthread.c\n> +++ b/compat/win32/pthread.c\n> @@ -56,3 +56,25 @@ pthread_t pthread_self(void)\n> t.tid = GetCurrentThreadId();\n> return t;\n> }\n> +\n> +/* Adapted from libav's compat/w32pthreads.h. */\n> +int pthread_once(pthread_once_t *once_control, void (*init_routine)(void))\n> +{\n> + BOOL pending = FALSE;\n> + int ret = 0;\n> +\n> + if(!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {\n\nWe put a space between \"if\" and the following \"(\". It might also be\ninteresting to know perhaps in the commit message how much you adapted\nthe code.\n\nFor example perhaps a good strategy would be in the commit that\nimports the code to do the minimal amount of change so that it builds\nand passes the test, and then to have another commit that adapts the\nstyle of the code.\n\n> + ret = err_win_to_posix(GetLastError());\n> + goto out;\n> + }\n> +\n> + if (pending)\n> + init_routine();\n> +\n> + if(!InitOnceComplete(once_control, 0, NULL))\n\nSpace missing between \"if\" and the following \"(\".\n\n> + ret = err_win_to_posix(GetLastError());\n> +\n> +out:\n> + /* POSIX doesn't allow pthread_once() to return EINTR */\n> + return ret == EINTR ? EIO : ret;\n> +}\n"},{"id":"400653","messageId":"CAP8UFD1yE8HRxEdw72T70Jw0Eb4RWDze0pD6sNUfF4SKMYuyWw@mail.gmail.com","threadId":"53752","inReplyTo":"0104cd9c763aee220a2df357834c79b10695ee35.1593115455.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-06-26T06:02:57Z","receivedAt":"2020-06-26T06:03:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares\n wrote:\n\n[...]\n\n> Although these functions don't seem to be causing problems out there for\n> now (at least not reported), making them thread-safe makes the codebase\n> more robust against race conditions. We can easily do that replicating\n\nMaybe s/that replicating/that by replicating/.\n\n> the static buffer in each thread's local storage.\n>\n> Original-patch-by: Fredrik Kuivinen \n\nIf Fredrik gave his \"Signed-off-by:\", maybe it's better to keep it too.\n\n> Signed-off-by: Matheus Tavares \n> ---\n"},{"id":"400658","messageId":"CAP8UFD0oWuoYLwgYLbkSvDjV1Ymedd_E2j8iv3QGGitgRVq6=Q@mail.gmail.com","threadId":"53752","inReplyTo":"cover.1593115455.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 0/2] Make oid_to_hex() thread-safe","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-06-26T08:22:41Z","receivedAt":"2020-06-26T08:22:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares\n wrote:\n>\n> Some thread-unsafe functions of our codebase appear very down in the\n> call stack, which can be hard to notice (or avoid). Thus they are\n> sometimes used in threaded code unsafely. In this series we add\n> pthread_once() to compat/win32/ and use it in conjunction with\n> pthread_key to make a subset of the said functions thread-safe.\n\nGreat!\n\n> As a next step, I would love to make [warning|error|die]_errno()\n> thread-safe as well. strerror() is not safe on *nix, and there are some\n> thread functions today that call these (although the actual risk of a\n> race condition must be very small...)\n>\n> My idea was to implement a xstrerror() wrapper which calls the\n> appropriate thread-safe function (dependant on the OS),\n\nYeah, that works if there are appropriate thread-safe functions for\nall the OS we are interested in, or if we can fallback to strerror()\nor calling it holding a lock.\n\n> or even call\n> strerror() itself but holding a lock to copy the result for a local\n> buffer (which should be OK as we don't expect contention in strerror).\n\nI agree that it should be ok.\n\n> We could also set a thread local buffer array, as in the second patch of\n> this series, to excuse callers from allocating/freeing memory.\n\nI don't think caller allocating/freeing memory for error strings is a\nperformance or code simplification issue.\n\n> One concern with this idea is the risk of an infinite recursion if\n> xstrerror() or any of its childs call [warning|error|die]_errno().\n> However, if we are only using strerror() and pthread_*() within the\n> wrapper, there should be no problem, right?\n\nYeah, I agree.\n\n> Has anyone thought of\n> other problems with this approach?\n>\n> Finally, should such change also come with a coccinelle patch to replace\n> usages of strerror() with xstrerror()? Or better not, as the change\n> would be too big?\n\nI would agree that it's better to avoid too big changes. I am not sure\nhow much we want to automate and check that though.\n\nThanks,\nChristian.\n"},{"id":"400685","messageId":"CAHd-oW7Ev_4=Kw8z=qi6U_oR6a7dzhL3kTH=xkL1PFsfqXu5QQ@mail.gmail.com","threadId":"53752","inReplyTo":"CAP8UFD3NwKtWYL_H5fWjT-HQVd3W8ZJ49p+faDyMQ+==ttCWjA@mail.gmail.com","subject":"Re: [PATCH 1/2] compat/win32/pthread: add pthread_once()","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-26T14:13:07Z","receivedAt":"2020-06-26T14:13:24Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Jun 26, 2020 at 2:45 AM Christian Couder\n wrote:\n>\n> On Fri, Jun 26, 2020 at 3:00 AM Matheus Tavares\n> wrote:\n> >\n> > Signed-off-by: Matheus Tavares \n> > ---\n>\n> The commit message might want to explain a bit the purpose of adding\n> these features.\n\nWill do!\n\n> > Note: the pthread_once() function is adapted from:\n> > https://git.libav.org/?p=libav.git;a=commitdiff;h=b22693b06d1e5d73454a65c203b4d31c1ca5b69a\n> >\n> > Which is LGPLv2.1. Should I add any notice/acknowledgment somewhere,\n> > besides the comment I added above the function?\n>\n> Yeah, I think you should also tell in the commit message where the\n> code comes from (along with the hash of the commit) and that libav is\n> LGPLv2.1 which is compatible with GPLv2 as explained in section 3 of\n> the LGPLv2.1.\n\nSounds good, I'll add that information.\n\n> > compat/win32/pthread.c | 22 ++++++++++++++++++++++\n> > compat/win32/pthread.h | 5 +++++\n> > thread-utils.c | 11 +++++++++++\n> > thread-utils.h | 6 ++++++\n> > 4 files changed, 44 insertions(+)\n> >\n> > diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\n> > index 2e7eead42c..5a7ecbd999 100644\n> > --- a/compat/win32/pthread.c\n> > +++ b/compat/win32/pthread.c\n> > @@ -56,3 +56,25 @@ pthread_t pthread_self(void)\n> > t.tid = GetCurrentThreadId();\n> > return t;\n> > }\n> > +\n> > +/* Adapted from libav's compat/w32pthreads.h. */\n> > +int pthread_once(pthread_once_t *once_control, void (*init_routine)(void))\n> > +{\n> > + BOOL pending = FALSE;\n> > + int ret = 0;\n> > +\n> > + if(!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {\n>\n> We put a space between \"if\" and the following \"(\". It might also be\n> interesting to know perhaps in the commit message how much you adapted\n> the code.\n>\n> For example perhaps a good strategy would be in the commit that\n> imports the code to do the minimal amount of change so that it builds\n> and passes the test, and then to have another commit that adapts the\n> style of the code.\n\nMakes sense. Alternatively, since the code ported from libav is quite\nsmall, I think we might as well already adapt the style in this same\ncommit. I'll fix that for v2, thanks!\n"},{"id":"400706","messageId":"CAHd-oW4bgEv7w2e-wD5aqje_esHA8-_ZgR9trbJXYJ8-M_4F4g@mail.gmail.com","threadId":"53752","inReplyTo":"CAP8UFD0oWuoYLwgYLbkSvDjV1Ymedd_E2j8iv3QGGitgRVq6=Q@mail.gmail.com","subject":"Re: [PATCH 0/2] Make oid_to_hex() thread-safe","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-26T16:22:52Z","receivedAt":"2020-06-26T16:23:07Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Christian\n\nThanks for the feedback!\n\nOn Fri, Jun 26, 2020 at 5:22 AM Christian Couder\n wrote:\n>\n> On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares\n> wrote:\n> >\n> > My idea was to implement a xstrerror() wrapper which calls the\n> > appropriate thread-safe function (dependant on the OS),\n[...]\n> > We could also set a thread local buffer array, as in the second patch of\n> > this series, to excuse callers from allocating/freeing memory.\n>\n> I don't think caller allocating/freeing memory for error strings is a\n> performance or code simplification issue.\n\nYeah, I agree that passing a buffer to xstrerror() should be fine. The\nproblem is that we already have many uses of strerror() (98, according\nto git-grep), which expect a static buffer in return. So the change\nwould be too big :(\n\n> > Finally, should such change also come with a coccinelle patch to replace\n> > usages of strerror() with xstrerror()? Or better not, as the change\n> > would be too big?\n>\n> I would agree that it's better to avoid too big changes. I am not sure\n> how much we want to automate and check that though.\n\nAnother alternative would be to replace the definition of strerror() with a:\n\n#define strerror(errnum) xstrerror(errnum)\n\nThis way, all the 98 current strerror() callers would start using the\nthread-safe version without the need to change them.... However, I\ndon't think I've ever seen such a replacement in our codebase before\n(besides the banned functions in banned.h). Also, people might expect\nstrerror() to refer to the system version, and perhaps make wrong\nassumptions about it? I'm not sure which is the best alternative, it's\na tricky issue :(\n"},{"id":"400721","messageId":"cover.1593208411.git.matheus.bernardino@usp.br","threadId":"53752","inReplyTo":"cover.1593115455.git.matheus.bernardino@usp.br","subject":"[PATCH v2 0/2] Make oid_to_hex() thread-safe","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-26T21:54:01Z","receivedAt":"2020-06-26T21:54:16Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Some thread-unsafe functions of our codebase appear very down in the\ncall stack, which can be hard to notice (or avoid). Thus they are\nsometimes used in threaded code unsafely. In this series we add\npthread_once() to compat/win32/ and use it in conjunction with\npthread_key to make a subset of the said functions thread-safe.\n\nChanges since v1:\n\nPatch 1:\n- Fixed style problems.\n- Wrote a more descriptive commit message, with motivation and info\n about where the code was ported from (including commit, license and\n modifications made).\n- Corrected usage of InitOnceComplete() in pthread_once(). It was\n previously being called unconditionally, which lead to errors in\n subsequent calls to pthread_once(), after the initialization had\n already succeded.\n\nPatch 2:\n- Used the correct pthread_[get|set]specific() functions in hex.c, as\n Brian pointed out.\n- Imported thread_utils.h to hex.c (providing the HAVE_THREADS macro)\n- Added Fredrik's S-o-B\n- Fixed spelling error\n- Added the static identifier to init_hexbuf_array_key() in hex.c\n- Provided an example in the commit message where oid_to_hex() was being\n used unsafelly.\n\nMatheus Tavares (2):\n compat/win32/pthread: add pthread_once()\n hex: make hash_to_hex_algop() and friends thread-safe\n\n compat/win32/pthread.c | 18 +++++++++++++++++\n compat/win32/pthread.h | 5 +++++\n hex.c | 46 ++++++++++++++++++++++++++++++++++++++----\n thread-utils.c | 11 ++++++++++\n thread-utils.h | 7 +++++++\n 5 files changed, 83 insertions(+), 4 deletions(-)\n\nRange-diff against v1:\n1: e5a10d3f21 ! 1: 783fcddf8d compat/win32/pthread: add pthread_once()\n @@ Metadata\n ## Commit message ##\n compat/win32/pthread: add pthread_once()\n \n + Add pthread_once() emulation for Windows. This function provides an easy\n + way to do thread-safe one-time initializations in multithreaded code. It\n + will be used in the next commit to make hash_to_hex_algop()\n + thread-safe.\n +\n + The pthread_once() implementation added comes from libav\n + (https://git.libav.org/?p=libav.git), commit b22693b (\"w32pthreads: Add\n + pthread_once emulation\", 2015-10-07). The code is licensed under\n + LGPLv2.1 which is compatible with GPLv2. Only the section for support on\n + Windows Vista+ has been ported, as that's the minimum version required\n + to build Git for Windows. Also, the code was modified to (1) check and\n + return error codes; and (2) do not call InitOnceComplete() again after a\n + successful initialization, as that results in an error.\n +\n Signed-off-by: Matheus Tavares \n \n ## compat/win32/pthread.c ##\n @@ compat/win32/pthread.c: pthread_t pthread_self(void)\n +\tBOOL pending = FALSE;\n +\tint ret = 0;\n +\n -+\tif(!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {\n ++\tif (!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {\n +\t\tret = err_win_to_posix(GetLastError());\n -+\t\tgoto out;\n -+\t}\n -+\n -+\tif (pending)\n ++\t} else if (pending) {\n +\t\tinit_routine();\n ++\t\tif (!InitOnceComplete(once_control, 0, NULL))\n ++\t\t\tret = err_win_to_posix(GetLastError());\n ++\t}\n +\n -+\tif(!InitOnceComplete(once_control, 0, NULL))\n -+\t\tret = err_win_to_posix(GetLastError());\n -+\n -+out:\n +\t/* POSIX doesn't allow pthread_once() to return EINTR */\n +\treturn ret == EINTR ? EIO : ret;\n +}\n @@ thread-utils.h: int dummy_pthread_join(pthread_t pthread, void **retval);\n int dummy_pthread_init(void *);\n \n +#define PTHREAD_ONCE_INIT 0\n ++#define pthread_once(once, routine) nothreads_pthread_once(once, routine)\n ++\n +int nothreads_pthread_once(pthread_once_t *once_control,\n +\t\t\t void (*init_routine)(void));\n -+#define pthread_once(once, routine) nothreads_pthread_once(once, routine)\n +\n #endif\n \n2: 0104cd9c76 ! 2: b47445fa1c hex: make hash_to_hex_algop() and friends thread-safe\n @@ Commit message\n in threaded code, but they are sometimes too deep in the call stack to\n be noticed or even avoided.\n \n - For example, we can take a look at the number of oid_to_hex() calls,\n - which calls hash_to_hex_algop():\n + grep.c:grep_source_load_oid(), for example, uses the thread-unsafe\n + oid_to_hex() (on errors) despite being called in threaded code. And\n + oid_to_hex() -- which calls hash_to_hex_algop() -- is used in many other\n + places, as well:\n \n $ git grep 'oid_to_hex(' | wc -l\n 818\n \n - Although these functions don't seem to be causing problems out there for\n - now (at least not reported), making them thread-safe makes the codebase\n - more robust against race conditions. We can easily do that replicating\n - the static buffer in each thread's local storage.\n + Although hash_to_hex_algop() and its wrappers don't seem to be causing\n + problems out there for now (at least not reported), making them\n + thread-safe makes the codebase more robust against race conditions. We\n + can easily do that by replicating the static buffer in each thread's\n + local storage.\n \n Original-patch-by: Fredrik Kuivinen \n + Signed-off-by: Fredrik Kuivinen \n Signed-off-by: Matheus Tavares \n \n ## hex.c ##\n +@@\n + #include \"cache.h\"\n ++#include \"thread-utils.h\"\n + \n + const signed char hexval_table[256] = {\n + \t -1, -1, -1, -1, -1, -1, -1, -1,\t\t/* 00-07 */\n @@ hex.c: char *oid_to_hex_r(char *buffer, const struct object_id *oid)\n \treturn hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);\n }\n @@ hex.c: char *oid_to_hex_r(char *buffer, const struct object_id *oid)\n +static pthread_key_t hexbuf_array_key;\n +static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT;\n +\n -+void init_hexbuf_array_key(void)\n ++static void init_hexbuf_array_key(void)\n +{\n +\tif (pthread_key_create(&hexbuf_array_key, free))\n +\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n @@ hex.c: char *oid_to_hex_r(char *buffer, const struct object_id *oid)\n +\tif (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n +\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n +\n -+\tvalue = pthread_key_getspecific(&hexbuf_array_key);\n ++\tvalue = pthread_getspecific(hexbuf_array_key);\n +\tif (value) {\n +\t\tha = (struct hexbuf_array *) value;\n +\t} else {\n +\t\tha = xmalloc(sizeof(*ha));\n -+\t\tif (pthread_key_setspecific(&hexbuf_array_key, (void *)ha))\n ++\t\tif (pthread_setspecific(hexbuf_array_key, (void *)ha))\n +\t\t\tdie(_(\"failed to set thread buffer for hash to hex conversion\"));\n +\t}\n +#else\n-- \n2.26.2\n\n"},{"id":"400722","messageId":"783fcddf8db92805ecab2d209d04466d22312075.1593208411.git.matheus.bernardino@usp.br","threadId":"53752","inReplyTo":"cover.1593208411.git.matheus.bernardino@usp.br","subject":"[PATCH v2 1/2] compat/win32/pthread: add pthread_once()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-26T21:54:02Z","receivedAt":"2020-06-26T21:54:18Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Add pthread_once() emulation for Windows. This function provides an easy\nway to do thread-safe one-time initializations in multithreaded code. It\nwill be used in the next commit to make hash_to_hex_algop()\nthread-safe.\n\nThe pthread_once() implementation added comes from libav\n(https://git.libav.org/?p=libav.git), commit b22693b (\"w32pthreads: Add\npthread_once emulation\", 2015-10-07). The code is licensed under\nLGPLv2.1 which is compatible with GPLv2. Only the section for support on\nWindows Vista+ has been ported, as that's the minimum version required\nto build Git for Windows. Also, the code was modified to (1) check and\nreturn error codes; and (2) do not call InitOnceComplete() again after a\nsuccessful initialization, as that results in an error.\n\nSigned-off-by: Matheus Tavares \n---\n compat/win32/pthread.c | 18 ++++++++++++++++++\n compat/win32/pthread.h | 5 +++++\n thread-utils.c | 11 +++++++++++\n thread-utils.h | 7 +++++++\n 4 files changed, 41 insertions(+)\n\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nindex 2e7eead42c..cb32f8c504 100644\n--- a/compat/win32/pthread.c\n+++ b/compat/win32/pthread.c\n@@ -56,3 +56,21 @@ pthread_t pthread_self(void)\n \tt.tid = GetCurrentThreadId();\n \treturn t;\n }\n+\n+/* Adapted from libav's compat/w32pthreads.h. */\n+int pthread_once(pthread_once_t *once_control, void (*init_routine)(void))\n+{\n+\tBOOL pending = FALSE;\n+\tint ret = 0;\n+\n+\tif (!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {\n+\t\tret = err_win_to_posix(GetLastError());\n+\t} else if (pending) {\n+\t\tinit_routine();\n+\t\tif (!InitOnceComplete(once_control, 0, NULL))\n+\t\t\tret = err_win_to_posix(GetLastError());\n+\t}\n+\n+\t/* POSIX doesn't allow pthread_once() to return EINTR */\n+\treturn ret == EINTR ? EIO : ret;\n+}\ndiff --git a/compat/win32/pthread.h b/compat/win32/pthread.h\nindex 737983d00b..c50f1e89c7 100644\n--- a/compat/win32/pthread.h\n+++ b/compat/win32/pthread.h\n@@ -40,6 +40,11 @@ typedef int pthread_mutexattr_t;\n #define pthread_cond_signal WakeConditionVariable\n #define pthread_cond_broadcast WakeAllConditionVariable\n \n+#define pthread_once_t INIT_ONCE\n+\n+#define PTHREAD_ONCE_INIT INIT_ONCE_STATIC_INIT\n+int pthread_once(pthread_once_t *once_control, void (*init_routine)(void));\n+\n /*\n * Simple thread creation implementation using pthread API\n */\ndiff --git a/thread-utils.c b/thread-utils.c\nindex 5329845691..937deb3f2e 100644\n--- a/thread-utils.c\n+++ b/thread-utils.c\n@@ -122,4 +122,15 @@ int dummy_pthread_join(pthread_t pthread, void **retval)\n \treturn ENOSYS;\n }\n \n+int nothreads_pthread_once(pthread_once_t *once_control,\n+\t\t\t void (*init_routine)(void))\n+{\n+\tif (*once_control == 1)\n+\t\treturn 0;\n+\n+\tinit_routine();\n+\t*once_control = 1;\n+\treturn 0;\n+}\n+\n #endif\ndiff --git a/thread-utils.h b/thread-utils.h\nindex 4961487ed9..8f9786217a 100644\n--- a/thread-utils.h\n+++ b/thread-utils.h\n@@ -19,6 +19,7 @@\n #define pthread_mutex_t int\n #define pthread_cond_t int\n #define pthread_key_t int\n+#define pthread_once_t int\n \n #define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)\n #define pthread_mutex_lock(mutex)\n@@ -48,6 +49,12 @@ int dummy_pthread_join(pthread_t pthread, void **retval);\n \n int dummy_pthread_init(void *);\n \n+#define PTHREAD_ONCE_INIT 0\n+#define pthread_once(once, routine) nothreads_pthread_once(once, routine)\n+\n+int nothreads_pthread_once(pthread_once_t *once_control,\n+\t\t\t void (*init_routine)(void));\n+\n #endif\n \n int online_cpus(void);\n-- \n2.26.2\n\n"},{"id":"400723","messageId":"b47445fa1cef6d4523dd0ca336f7ee22bce89466.1593208411.git.matheus.bernardino@usp.br","threadId":"53752","inReplyTo":"cover.1593208411.git.matheus.bernardino@usp.br","subject":"[PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-26T21:54:03Z","receivedAt":"2020-06-26T21:54:23Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"hash_to_hex_algop() returns a static buffer, relieving callers from the\nresponsibility of freeing memory after use. But the current\nimplementation uses the same static data for all threads and, thus, is\nnot thread-safe. We could avoid using this function and its wrappers\nin threaded code, but they are sometimes too deep in the call stack to\nbe noticed or even avoided.\n\ngrep.c:grep_source_load_oid(), for example, uses the thread-unsafe\noid_to_hex() (on errors) despite being called in threaded code. And\noid_to_hex() -- which calls hash_to_hex_algop() -- is used in many other\nplaces, as well:\n\n$ git grep 'oid_to_hex(' | wc -l\n818\n\nAlthough hash_to_hex_algop() and its wrappers don't seem to be causing\nproblems out there for now (at least not reported), making them\nthread-safe makes the codebase more robust against race conditions. We\ncan easily do that by replicating the static buffer in each thread's\nlocal storage.\n\nOriginal-patch-by: Fredrik Kuivinen \nSigned-off-by: Fredrik Kuivinen \nSigned-off-by: Matheus Tavares \n---\n hex.c | 46 ++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 42 insertions(+), 4 deletions(-)\n\ndiff --git a/hex.c b/hex.c\nindex da51e64929..4f2f163d5e 100644\n--- a/hex.c\n+++ b/hex.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"thread-utils.h\"\n \n const signed char hexval_table[256] = {\n \t -1, -1, -1, -1, -1, -1, -1, -1,\t\t/* 00-07 */\n@@ -136,12 +137,49 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)\n \treturn hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);\n }\n \n+struct hexbuf_array {\n+\tint idx;\n+\tchar bufs[4][GIT_MAX_HEXSZ + 1];\n+};\n+\n+#ifdef HAVE_THREADS\n+static pthread_key_t hexbuf_array_key;\n+static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT;\n+\n+static void init_hexbuf_array_key(void)\n+{\n+\tif (pthread_key_create(&hexbuf_array_key, free))\n+\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n+}\n+\n+#else\n+static struct hexbuf_array default_hexbuf_array;\n+#endif\n+\n char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *algop)\n {\n-\tstatic int bufno;\n-\tstatic char hexbuffer[4][GIT_MAX_HEXSZ + 1];\n-\tbufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);\n-\treturn hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);\n+\tstruct hexbuf_array *ha;\n+\n+#ifdef HAVE_THREADS\n+\tvoid *value;\n+\n+\tif (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n+\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n+\n+\tvalue = pthread_getspecific(hexbuf_array_key);\n+\tif (value) {\n+\t\tha = (struct hexbuf_array *) value;\n+\t} else {\n+\t\tha = xmalloc(sizeof(*ha));\n+\t\tif (pthread_setspecific(hexbuf_array_key, (void *)ha))\n+\t\t\tdie(_(\"failed to set thread buffer for hash to hex conversion\"));\n+\t}\n+#else\n+\tha = &default_hexbuf_array;\n+#endif\n+\n+\tha->idx = (ha->idx + 1) % ARRAY_SIZE(ha->bufs);\n+\treturn hash_to_hex_algop_r(ha->bufs[ha->idx], hash, algop);\n }\n \n char *hash_to_hex(const unsigned char *hash)\n-- \n2.26.2\n\n"},{"id":"400814","messageId":"nycvar.QRO.7.76.6.2006291646420.56@tvgsbejvaqbjf.bet","threadId":"53752","inReplyTo":"b47445fa1cef6d4523dd0ca336f7ee22bce89466.1593208411.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-06-29T15:11:59Z","receivedAt":"2020-06-30T15:01:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Matheus,\n\nI am fine with the Windows changes (although I have to admit that I did\nnot find time to test things yet).\n\nThere is one problem in that I do not necessarily know that the memory is\nreleased correctly when threads end; You will notice that the\n`pthread_key_create()` shim in `compat/win32/pthread.h` does not use the\n`destructor` parameter at all. The documentation at\n\n\thttps://docs.microsoft.com/en-us/windows/win32/procthread/using-thread-local-storage\n\nis also not terribly clear _how_ the memory is released that was assigned\nvia `TlsSetValue()`. I notice that the example uses `LocalAlloc()`, but we\noverride `malloc()` via the `compat/nedmalloc/` functions, so that should\ncause problems unless I am wrong.\n\nMaybe there is an expert reading this who could jump in and help\nunderstand the ramifications?\n\nA couple more things:\n\nOn Fri, 26 Jun 2020, Matheus Tavares wrote:\n\n> hash_to_hex_algop() returns a static buffer, relieving callers from the\n> responsibility of freeing memory after use. But the current\n> implementation uses the same static data for all threads and, thus, is\n> not thread-safe. We could avoid using this function and its wrappers\n> in threaded code, but they are sometimes too deep in the call stack to\n> be noticed or even avoided.\n>\n> grep.c:grep_source_load_oid(), for example, uses the thread-unsafe\n> oid_to_hex() (on errors) despite being called in threaded code. And\n> oid_to_hex() -- which calls hash_to_hex_algop() -- is used in many other\n> places, as well:\n>\n> $ git grep 'oid_to_hex(' | wc -l\n> 818\n>\n> Although hash_to_hex_algop() and its wrappers don't seem to be causing\n> problems out there for now (at least not reported), making them\n> thread-safe makes the codebase more robust against race conditions. We\n> can easily do that by replicating the static buffer in each thread's\n> local storage.\n>\n> Original-patch-by: Fredrik Kuivinen \n> Signed-off-by: Fredrik Kuivinen \n> Signed-off-by: Matheus Tavares \n> ---\n> hex.c | 46 ++++++++++++++++++++++++++++++++++++++++++----\n> 1 file changed, 42 insertions(+), 4 deletions(-)\n>\n> diff --git a/hex.c b/hex.c\n> index da51e64929..4f2f163d5e 100644\n> --- a/hex.c\n> +++ b/hex.c\n> @@ -1,4 +1,5 @@\n> #include \"cache.h\"\n> +#include \"thread-utils.h\"\n>\n> const signed char hexval_table[256] = {\n> \t -1, -1, -1, -1, -1, -1, -1, -1,\t\t/* 00-07 */\n> @@ -136,12 +137,49 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)\n> \treturn hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);\n> }\n>\n> +struct hexbuf_array {\n> +\tint idx;\n\nIs there a specific reason why you renamed `bufno` to `idx`? If not, I'd\nrather keep the old name.\n\n> +\tchar bufs[4][GIT_MAX_HEXSZ + 1];\n> +};\n> +\n> +#ifdef HAVE_THREADS\n> +static pthread_key_t hexbuf_array_key;\n> +static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT;\n> +\n> +static void init_hexbuf_array_key(void)\n> +{\n> +\tif (pthread_key_create(&hexbuf_array_key, free))\n> +\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> +}\n> +\n> +#else\n> +static struct hexbuf_array default_hexbuf_array;\n> +#endif\n> +\n> char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *algop)\n> {\n> -\tstatic int bufno;\n> -\tstatic char hexbuffer[4][GIT_MAX_HEXSZ + 1];\n> -\tbufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);\n> -\treturn hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);\n> +\tstruct hexbuf_array *ha;\n> +\n> +#ifdef HAVE_THREADS\n> +\tvoid *value;\n> +\n> +\tif (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n> +\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> +\n> +\tvalue = pthread_getspecific(hexbuf_array_key);\n> +\tif (value) {\n> +\t\tha = (struct hexbuf_array *) value;\n> +\t} else {\n> +\t\tha = xmalloc(sizeof(*ha));\n> +\t\tif (pthread_setspecific(hexbuf_array_key, (void *)ha))\n> +\t\t\tdie(_(\"failed to set thread buffer for hash to hex conversion\"));\n> +\t}\n> +#else\n> +\tha = &default_hexbuf_array;\n> +#endif\n\nThis introduces two ugly `#ifdef HAVE_THREADS` constructs which are\nproblematic because they are the most likely places to introduce compile\nerrors.\n\nI wonder whether you considered introducing a function (and probably a\nmacro) that transparently gives you a thread-specific instance of a given\ndata type? The caller would look something like\n\n\tstruct hexbuf_array *hex_array;\n\n\tGET_THREADSPECIFIC(hex_array);\n\nwhere the macro would look somewhat like this:\n\n\t#define GET_THREADSPECIFIC(var) \\\n\t\tif (get_thread_specific(&var, sizeof(var)) < 0)\n\t\t\tdie(_(\"Failed to get thread-specific %s\"), #var);\n\nand the function would allocate and assign the variable. I guess this\nscheme won't work, though, as `pthread_once()` does not take a callback\nparameter, right? And we would need that parameter to be able to create\nthe `pthread_key_t`. Hmm.\n\nCiao,\nDscho\n\n> +\n> +\tha->idx = (ha->idx + 1) % ARRAY_SIZE(ha->bufs);\n> +\treturn hash_to_hex_algop_r(ha->bufs[ha->idx], hash, algop);\n> }\n>\n> char *hash_to_hex(const unsigned char *hash)\n> --\n> 2.26.2\n>\n>\n"},{"id":"400852","messageId":"CAHd-oW6U9krWYd++5ipGA6e4Xy6ZoBxE+QVFpshrjUKjN+RH0g@mail.gmail.com","threadId":"53752","inReplyTo":"nycvar.QRO.7.76.6.2006291646420.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-06-30T20:37:30Z","receivedAt":"2020-06-30T20:37:48Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Jun 30, 2020 at 12:01 PM Johannes Schindelin\n wrote:\n>\n> Hi Matheus,\n>\n> I am fine with the Windows changes (although I have to admit that I did\n> not find time to test things yet).\n>\n> There is one problem in that I do not necessarily know that the memory is\n> released correctly when threads end; You will notice that the\n> `pthread_key_create()` shim in `compat/win32/pthread.h` does not use the\n> `destructor` parameter at all. The documentation at\n>\n> https://docs.microsoft.com/en-us/windows/win32/procthread/using-thread-local-storage\n>\n> is also not terribly clear _how_ the memory is released that was assigned\n> via `TlsSetValue()`.\n\nYes, I wasn't sure about that either... It would be great if someone\nfamiliar with TLS memory on Windows could help us with this.\n\n> A couple more things:\n>\n> On Fri, 26 Jun 2020, Matheus Tavares wrote:\n> >\n[...]\n> > +struct hexbuf_array {\n> > + int idx;\n>\n> Is there a specific reason why you renamed `bufno` to `idx`? If not, I'd\n> rather keep the old name.\n\nOK, I'll change it back in v3.\n\n> > + char bufs[4][GIT_MAX_HEXSZ + 1];\n> > +};\n> > +\n> > +#ifdef HAVE_THREADS\n> > +static pthread_key_t hexbuf_array_key;\n> > +static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT;\n> > +\n> > +static void init_hexbuf_array_key(void)\n> > +{\n> > + if (pthread_key_create(&hexbuf_array_key, free))\n> > + die(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> > +}\n> > +\n> > +#else\n> > +static struct hexbuf_array default_hexbuf_array;\n> > +#endif\n> > +\n> > char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *algop)\n> > {\n> > - static int bufno;\n> > - static char hexbuffer[4][GIT_MAX_HEXSZ + 1];\n> > - bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);\n> > - return hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);\n> > + struct hexbuf_array *ha;\n> > +\n> > +#ifdef HAVE_THREADS\n> > + void *value;\n> > +\n> > + if (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n> > + die(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> > +\n> > + value = pthread_getspecific(hexbuf_array_key);\n> > + if (value) {\n> > + ha = (struct hexbuf_array *) value;\n> > + } else {\n> > + ha = xmalloc(sizeof(*ha));\n> > + if (pthread_setspecific(hexbuf_array_key, (void *)ha))\n> > + die(_(\"failed to set thread buffer for hash to hex conversion\"));\n> > + }\n> > +#else\n> > + ha = &default_hexbuf_array;\n> > +#endif\n>\n> This introduces two ugly `#ifdef HAVE_THREADS` constructs which are\n> problematic because they are the most likely places to introduce compile\n> errors.\n>\n> I wonder whether you considered introducing a function (and probably a\n> macro) that transparently gives you a thread-specific instance of a given\n> data type? The caller would look something like\n>\n> struct hexbuf_array *hex_array;\n>\n> GET_THREADSPECIFIC(hex_array);\n>\n> where the macro would look somewhat like this:\n>\n> #define GET_THREADSPECIFIC(var) \\\n> if (get_thread_specific(&var, sizeof(var)) < 0)\n> die(_(\"Failed to get thread-specific %s\"), #var);\n>\n> and the function would allocate and assign the variable.\n\nHmm, we would need two macros, wouldn't we? GET_THREADSPECIFIC(var)\nand a DECLARE_THREADSPECIFIC(var, destructor), to declare the\npthread_once_t and pthread_key_t variables, as well as define a\ninitialization function for the key (i.e. the callback to\npthread_once()). Or we could provide these declarations ourselves, but\nthen we would need the \"ifdef HAVE_THREADS\" guard to avoid calling\npthread_key_create() when there is no multithreading.\n\nI think that would work, yes. Alternatively, I think we could adjust\nthe dummy pthread_key_* functions in thread-utils.h to emulate the\nreal ones when HAVE_THREADS == false. Then we could eliminate the\n`#ifdef HAVE_THREADS` guards and use the same code for both cases here\n(and everywhere else pthread_key is used). I haven't thought about it\ncarefully enough yet, but I think this *might* be as simple as adding\nthe following defines in the \"#ifdef NO_PTHREADS\" section of\nthread-utils.h:\n\n#define pthread_key_t void *\n/*\n * The destructor is not used in this case as the main thread will only\n * exit when the program terminates.\n */\n#define pthread_key_create(key_ptr, unused) return_0((*key_ptr) = NULL)\n#define pthread_setspecific(key, value) return_0((key) = (value))\n#define pthread_getspecific(key) (key)\n#define pthread_key_delete(key) return_0(NULL)\n\nstatic inline int return_0(void *unused)\n{\n return 0;\n}\n\nThat's the general idea, but we might as well define a `struct\ndummy_pthread_key_t` instead of defining the key directly as `void *`\n(and have functions instead of macros). This way we could store, e.g.,\nan \"initialized\" flag that could be used to return an error code on\ndouble-initializations.\n\nWhat do you think?\n"},{"id":"400890","messageId":"nycvar.QRO.7.76.6.2007011244150.56@tvgsbejvaqbjf.bet","threadId":"53752","inReplyTo":"CAHd-oW6U9krWYd++5ipGA6e4Xy6ZoBxE+QVFpshrjUKjN+RH0g@mail.gmail.com","subject":"Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-01T11:32:57Z","receivedAt":"2020-07-01T11:33:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Matheus,\n\nOn Tue, 30 Jun 2020, Matheus Tavares Bernardino wrote:\n\n> On Tue, Jun 30, 2020 at 12:01 PM Johannes Schindelin\n> wrote:\n> >\n> > Hi Matheus,\n> >\n> > I am fine with the Windows changes (although I have to admit that I did\n> > not find time to test things yet).\n> >\n> > There is one problem in that I do not necessarily know that the memory is\n> > released correctly when threads end; You will notice that the\n> > `pthread_key_create()` shim in `compat/win32/pthread.h` does not use the\n> > `destructor` parameter at all. The documentation at\n> >\n> > https://docs.microsoft.com/en-us/windows/win32/procthread/using-thread-local-storage\n> >\n> > is also not terribly clear _how_ the memory is released that was assigned\n> > via `TlsSetValue()`.\n>\n> Yes, I wasn't sure about that either... It would be great if someone\n> familiar with TLS memory on Windows could help us with this.\n\nFrom reading\nhttps://docs.microsoft.com/en-us/windows/win32/procthread/using-thread-local-storage,\nI get the impression that it is the duty of the thread function to take\ncare of releasing the memory.\n\nLooking at our code base, I do not see any obvious single point where we\ncould take care of that.\n\nAnd it does not look like there is a function you can register to be\ncalled just before threads shut down.\n\nHmm.\n\nA very involved solution would be to override our `pthread_create()`\nemulation to override the function being called, passing as function\nparameter a pointer to a struct containing the originally-specified thread\nfunction and parameter, and the overriding function would then call that\nfunction, save its return value, take care of releasing the memory, and\nreturn the saved return value.\n\nLikewise, our `pthread_setspecific()` shim in `compat/win32/pthread.h`\nwould need to learn to append the passed pointer to a list, and we should\nprobably also edit the `pthread_key_create()` shim to record the function\nto call when releasing the memory at the end of a thread.\n\nThis strikes me as pretty ugly and complex.\n\nYet when I read the LGPLv2'ed code of\nhttps://sourceware.org/pthreads-win32/ that is exactly what they use.\nSince their source code is still in CVS, and their anonymous CVS viewer no\nlonger works, I had to find a copy to point to:\nhttps://github.com/jingyu/pthreads-w32/blob/master/pthread_key_create.c\n\nSo we could now start to require a non-minimal pthreads emulation on\nWindows. I am somewhat opposed to that idea.\n\nNow I wonder just how involved it _really_ would be to convert all of\nthose `oid_to_hex()` calls into what is effectively an `_r()` variant,\nwhere you _have_ to pass a buffer to store the result. That would make it\na ton easier to keep Git portable.\n\nI do understand that 800+ hits for `oid_to_hex()` make this somewhat of a\npain to implement...\n\nMaybe there is a way to convince Coccinelle to do all that work for us?\n\n> > A couple more things:\n> >\n> > On Fri, 26 Jun 2020, Matheus Tavares wrote:\n> > >\n> [...]\n> > > +struct hexbuf_array {\n> > > + int idx;\n> >\n> > Is there a specific reason why you renamed `bufno` to `idx`? If not, I'd\n> > rather keep the old name.\n>\n> OK, I'll change it back in v3.\n>\n> > > + char bufs[4][GIT_MAX_HEXSZ + 1];\n> > > +};\n> > > +\n> > > +#ifdef HAVE_THREADS\n> > > +static pthread_key_t hexbuf_array_key;\n> > > +static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT;\n> > > +\n> > > +static void init_hexbuf_array_key(void)\n> > > +{\n> > > + if (pthread_key_create(&hexbuf_array_key, free))\n> > > + die(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> > > +}\n> > > +\n> > > +#else\n> > > +static struct hexbuf_array default_hexbuf_array;\n> > > +#endif\n> > > +\n> > > char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *algop)\n> > > {\n> > > - static int bufno;\n> > > - static char hexbuffer[4][GIT_MAX_HEXSZ + 1];\n> > > - bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);\n> > > - return hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);\n> > > + struct hexbuf_array *ha;\n> > > +\n> > > +#ifdef HAVE_THREADS\n> > > + void *value;\n> > > +\n> > > + if (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n> > > + die(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> > > +\n> > > + value = pthread_getspecific(hexbuf_array_key);\n> > > + if (value) {\n> > > + ha = (struct hexbuf_array *) value;\n> > > + } else {\n> > > + ha = xmalloc(sizeof(*ha));\n> > > + if (pthread_setspecific(hexbuf_array_key, (void *)ha))\n> > > + die(_(\"failed to set thread buffer for hash to hex conversion\"));\n> > > + }\n> > > +#else\n> > > + ha = &default_hexbuf_array;\n> > > +#endif\n> >\n> > This introduces two ugly `#ifdef HAVE_THREADS` constructs which are\n> > problematic because they are the most likely places to introduce compile\n> > errors.\n> >\n> > I wonder whether you considered introducing a function (and probably a\n> > macro) that transparently gives you a thread-specific instance of a given\n> > data type? The caller would look something like\n> >\n> > struct hexbuf_array *hex_array;\n> >\n> > GET_THREADSPECIFIC(hex_array);\n> >\n> > where the macro would look somewhat like this:\n> >\n> > #define GET_THREADSPECIFIC(var) \\\n> > if (get_thread_specific(&var, sizeof(var)) < 0)\n> > die(_(\"Failed to get thread-specific %s\"), #var);\n> >\n> > and the function would allocate and assign the variable.\n>\n> Hmm, we would need two macros, wouldn't we? GET_THREADSPECIFIC(var)\n> and a DECLARE_THREADSPECIFIC(var, destructor), to declare the\n> pthread_once_t and pthread_key_t variables, as well as define a\n> initialization function for the key (i.e. the callback to\n> pthread_once()). Or we could provide these declarations ourselves, but\n> then we would need the \"ifdef HAVE_THREADS\" guard to avoid calling\n> pthread_key_create() when there is no multithreading.\n\nRight. I wanted to avoid the need to call `DECLARE_THREADSPECIFIC()`, in\nparticular because that would have to be _outside_ of any function\n(because it has to define a function, and nested functions are not really\nsupported in C, at least not widely).\n\nCiao,\nDscho\n\n>\n> I think that would work, yes. Alternatively, I think we could adjust\n> the dummy pthread_key_* functions in thread-utils.h to emulate the\n> real ones when HAVE_THREADS == false. Then we could eliminate the\n> `#ifdef HAVE_THREADS` guards and use the same code for both cases here\n> (and everywhere else pthread_key is used). I haven't thought about it\n> carefully enough yet, but I think this *might* be as simple as adding\n> the following defines in the \"#ifdef NO_PTHREADS\" section of\n> thread-utils.h:\n>\n> #define pthread_key_t void *\n> /*\n> * The destructor is not used in this case as the main thread will only\n> * exit when the program terminates.\n> */\n> #define pthread_key_create(key_ptr, unused) return_0((*key_ptr) = NULL)\n> #define pthread_setspecific(key, value) return_0((key) = (value))\n> #define pthread_getspecific(key) (key)\n> #define pthread_key_delete(key) return_0(NULL)\n>\n> static inline int return_0(void *unused)\n> {\n> return 0;\n> }\n>\n> That's the general idea, but we might as well define a `struct\n> dummy_pthread_key_t` instead of defining the key directly as `void *`\n> (and have functions instead of macros). This way we could store, e.g.,\n> an \"initialized\" flag that could be used to return an error code on\n> double-initializations.\n>\n> What do you think?\n>\n"},{"id":"401633","messageId":"nycvar.QRO.7.76.6.2007161214270.54@tvgsbejvaqbjf.bet","threadId":"53752","inReplyTo":"b47445fa1cef6d4523dd0ca336f7ee22bce89466.1593208411.git.matheus.bernardino@usp.br","subject":"Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-07-16T11:29:52Z","receivedAt":"2020-07-16T12:56:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Matheus,\n\nOn Fri, 26 Jun 2020, Matheus Tavares wrote:\n\n> @@ -136,12 +137,49 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)\n> \treturn hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);\n> }\n>\n> +struct hexbuf_array {\n> +\tint idx;\n> +\tchar bufs[4][GIT_MAX_HEXSZ + 1];\n> +};\n> +\n> +#ifdef HAVE_THREADS\n> +static pthread_key_t hexbuf_array_key;\n> +static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT;\n> +\n> +static void init_hexbuf_array_key(void)\n> +{\n> +\tif (pthread_key_create(&hexbuf_array_key, free))\n> +\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> +}\n> +\n> +#else\n> +static struct hexbuf_array default_hexbuf_array;\n> +#endif\n> +\n> char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *algop)\n> {\n> -\tstatic int bufno;\n> -\tstatic char hexbuffer[4][GIT_MAX_HEXSZ + 1];\n> -\tbufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);\n> -\treturn hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);\n> +\tstruct hexbuf_array *ha;\n> +\n> +#ifdef HAVE_THREADS\n> +\tvoid *value;\n> +\n> +\tif (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))\n> +\t\tdie(_(\"failed to initialize threads' key for hash to hex conversion\"));\n> +\n> +\tvalue = pthread_getspecific(hexbuf_array_key);\n> +\tif (value) {\n> +\t\tha = (struct hexbuf_array *) value;\n> +\t} else {\n> +\t\tha = xmalloc(sizeof(*ha));\n\nI just realized (while trying to debug something independent) that this\nleaves `ha->idx` uninitialized. So you will need at least this patch to\nfix a bug that currently haunts `seen`'s CI builds (you can use\n`--valgrind`, like I did, to identify the problem):\n\n-- snip --\ndiff --git a/hex.c b/hex.c\nindex 4f2f163d5e7..365ba94ab11 100644\n--- a/hex.c\n+++ b/hex.c\n@@ -171,6 +171,7 @@ char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *a\n \t\tha = (struct hexbuf_array *) value;\n \t} else {\n \t\tha = xmalloc(sizeof(*ha));\n+\t\tha->idx = 0;\n \t\tif (pthread_setspecific(hexbuf_array_key, (void *)ha))\n \t\t\tdie(_(\"failed to set thread buffer for hash to hex conversion\"));\n \t}\n-- snap --\n\nBut as I mentioned before, I would be much more in favor of abandoning\nthis thread-local idea (because it is _still_ fragile, as the same thread\ncould try to make use of more than four hex values in the same `printf()`,\nfor example) and instead using Coccinelle to convert all those\n`oid_to_hex()` calls to `oid_to_hex_r()` calls.\n\nNow, I am _far_ from knowing what I'm doing with Coccinelle, but I think\nthis here semantic patch should get you going:\n\n-- snipsnap --\n@@\nexpression E;\n@@\n {\n++ char hex[GIT_MAX_HEXSZ + 1];\n ...\n- oid_to_hex(E)\n+ oid_to_hex_r(hex, E)\n ...\n }\n\n@@\nexpression E1, E2;\n@@\n {\n++ char hex1[GIT_MAX_HEXSZ + 1], hex2[GIT_MAX_HEXSZ + 1];\n ...\n- oid_to_hex(E1)\n+ oid_to_hex_r(hex1, E1)\n ...\n- oid_to_hex(E2)\n+ oid_to_hex_r(hex2, E2)\n ...\n }\n"},{"id":"401723","messageId":"CAHd-oW6mvaANTBX5sVEi49s_Ku71-ZXqhyePtkwbCKaTK7tm4Q@mail.gmail.com","threadId":"53752","inReplyTo":"nycvar.QRO.7.76.6.2007161214270.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-07-18T03:09:34Z","receivedAt":"2020-07-18T03:09:50Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Dscho\n\nOn Thu, Jul 16, 2020 at 9:56 AM Johannes Schindelin\n wrote:\n>\n> On Fri, 26 Jun 2020, Matheus Tavares wrote:\n>\n> > + value = pthread_getspecific(hexbuf_array_key);\n> > + if (value) {\n> > + ha = (struct hexbuf_array *) value;\n> > + } else {\n> > + ha = xmalloc(sizeof(*ha));\n>\n> I just realized (while trying to debug something independent) that this\n> leaves `ha->idx` uninitialized.\n\nThanks for catching that! I fixed it in my local branch.\n\n> But as I mentioned before, I would be much more in favor of abandoning\n> this thread-local idea (because it is _still_ fragile, as the same thread\n> could try to make use of more than four hex values in the same `printf()`,\n> for example) and instead using Coccinelle to convert all those\n> `oid_to_hex()` calls to `oid_to_hex_r()` calls.\n\nYeah, I agree that removing oid_to_hex() in favor of oid_to_hex_r()\nwould be great. Unfortunately, I only used Coccinelle for basic\nthings, such as function renaming. And I won't have the time to study\nit further at the moment :( Therefore, I think I'll ask Junio to drop\nthis series for now, until I or someone else finds some time to work\non the semantic patch.\n\nAlternatively, if using thread-local storage is still an option, I\nthink I might have solved the problems we had in the previous\niteration with memory leaks on Windows. I changed our\npthread_key_create() emulation to start using the destructor callback\non Windows, through the Fiber Local Storage (FLS) API. As the\ndocumentation says [1] \"If no fiber switching occurs, FLS acts exactly\nthe same as thread local storage\". The advantage over TLS is that\nFLSAlloc() does take a callback parameter.\n\nI also removed the ugly `#ifdef HAVE_THREADS` guards on the last\npatch, as you suggested, and added some tests for our pthread_key\nemulation. In case you want to take a look to see if it might be worth\npursuing this route, the patches are here:\nhttps://github.com/matheustavares/git/commits/safe_oid_to_hex_v3\n\n[1]: https://docs.microsoft.com/en-us/windows/win32/procthread/fibers#fiber-local-storage\n"},{"id":"401724","messageId":"20200718035201.42233-1-matheus.bernardino@usp.br","threadId":"53752","inReplyTo":"nycvar.QRO.7.76.6.2007161214270.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-07-18T03:52:01Z","receivedAt":"2020-07-18T03:52:13Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Jul 16, 2020 at 9:56 AM Johannes Schindelin wrote:\n>\n> Now, I am _far_ from knowing what I'm doing with Coccinelle, but I think\n> this here semantic patch should get you going:\n>\n> -- snipsnap --\n> @@\n> expression E;\n> @@\n>   {\n> ++   char hex[GIT_MAX_HEXSZ + 1];\n>      ...\n> -    oid_to_hex(E)\n> +    oid_to_hex_r(hex, E)\n>      ...\n>   }\n>\n> @@\n> expression E1, E2;\n> @@\n>   {\n> ++   char hex1[GIT_MAX_HEXSZ + 1], hex2[GIT_MAX_HEXSZ + 1];\n>      ...\n> -    oid_to_hex(E1)\n> +    oid_to_hex_r(hex1, E1)\n>      ...\n> -    oid_to_hex(E2)\n> +    oid_to_hex_r(hex2, E2)\n>      ...\n>   }\n\nThanks for this nice example! This already worked very well in some of\nmy tests :)\n\nHowever, with my _very_ limited notion of Coccinelle, I didn't\nunderstand why some code snippets didn't match the above rules. For\nexample, the structure below:\n\nfunc(...)\n{\n\tif (cond)\n\t\tfunc2(\"%s\", oid_to_hex(a));\n}\n\nI thought it could be because the `if` statement is missing the curly\nbrackets (and it does work if I add the brackets), but to my surprise,\nadding another oid_to_hex() call in an `else` case also made the code\nmatch the rule:\n\nfunc(...)\n{\n\tif (cond)\n\t\tfunc2(\"%s\", oid_to_hex(a));\n\telse\n\t\tfunc2(\"%s\", oid_to_hex(a));\n}\n\nThe following snippet also correctly matches, but spatch introduces only\none `hex` variable:\n\n\tif (cond)\n\t\tfunc2(\"%s, %s\", oid_to_hex(a), oid_to_hex(b));\n\telse\n\t\tfunc2(\"%s\", oid_to_hex(a));\n\nWe will probably want our semantic rules to handle an arbitrary number\nof `oid_to_hex()` calls in each function, but in scenarios like the\nabove one, we only really need 2 hex buffers despite having 3 calls...\nThat might be a little tricky, I guess.\n\nAnother thing that might be tricky in this conversion is checking for\nname conflicts with the added `hex` variable (but maybe Coccinelle\nalready has a facilitator mechanism for such cases? IDK).\n"},{"id":"402062","messageId":"a35a9334-6db2-d8e3-8ce5-15f15a9005e1@web.de","threadId":"53752","inReplyTo":"20200718035201.42233-1-matheus.bernardino@usp.br","subject":"Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-07-26T17:41:08Z","receivedAt":"2020-07-26T17:41:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.07.20 um 05:52 schrieb Matheus Tavares:\n> On Thu, Jul 16, 2020 at 9:56 AM Johannes Schindelin wrote:\n>>\n>> Now, I am _far_ from knowing what I'm doing with Coccinelle, but I think\n>> this here semantic patch should get you going:\n>>\n>> -- snipsnap --\n>> @@\n>> expression E;\n>> @@\n>>   {\n>> ++   char hex[GIT_MAX_HEXSZ + 1];\n>>      ...\n>> -    oid_to_hex(E)\n>> +    oid_to_hex_r(hex, E)\n>>      ...\n>>   }\n>>\n>> @@\n>> expression E1, E2;\n>> @@\n>>   {\n>> ++   char hex1[GIT_MAX_HEXSZ + 1], hex2[GIT_MAX_HEXSZ + 1];\n>>      ...\n>> -    oid_to_hex(E1)\n>> +    oid_to_hex_r(hex1, E1)\n>>      ...\n>> -    oid_to_hex(E2)\n>> +    oid_to_hex_r(hex2, E2)\n>>      ...\n>>   }\n>\n> Thanks for this nice example! This already worked very well in some of\n> my tests :)\n>\n> However, with my _very_ limited notion of Coccinelle, I didn't\n> understand why some code snippets didn't match the above rules. For\n> example, the structure below:\n>\n> func(...)\n> {\n> \tif (cond)\n> \t\tfunc2(\"%s\", oid_to_hex(a));\n> }\n>\n> I thought it could be because the `if` statement is missing the curly\n> brackets (and it does work if I add the brackets), but to my surprise,\n> adding another oid_to_hex() call in an `else` case also made the code\n> match the rule:\n>\n> func(...)\n> {\n> \tif (cond)\n> \t\tfunc2(\"%s\", oid_to_hex(a));\n> \telse\n> \t\tfunc2(\"%s\", oid_to_hex(a));\n> }\n>\n> The following snippet also correctly matches, but spatch introduces only\n> one `hex` variable:\n>\n> \tif (cond)\n> \t\tfunc2(\"%s, %s\", oid_to_hex(a), oid_to_hex(b));\n> \telse\n> \t\tfunc2(\"%s\", oid_to_hex(a));\n>\n\nThe produced diff looks like this:\n\n+\tchar hex[GIT_MAX_HEXSZ + 1];\n \tif (cond)\n-\t\tfunc2(\"%s, %s\", oid_to_hex(a), oid_to_hex(b));\n+\t\tfunc2(\"%s, %s\", oid_to_hex_r(hex, a), oid_to_hex_r(hex, b));\n \telse\n-\t\tfunc2(\"%s\", oid_to_hex(a));\n+\t\tfunc2(\"%s\", oid_to_hex_r(hex, a));\n\nSo the first rule was applied (the one for a single oid_to_hex call),\nbut we need the second one (for two oid_to_hex calls). Using the same\nhex buffer for two different values won't work.\n\n> We will probably want our semantic rules to handle an arbitrary number\n> of `oid_to_hex()` calls in each function, but in scenarios like the\n> above one, we only really need 2 hex buffers despite having 3 calls...\n\noid_to_hex() has two interesting features, and we need to make sure they\nare preserved for callers that use them: It has a ring of four buffers,\nso you can use it for four different values, and those buffers are\nstatic, so its results can be passed around arbitrarily.\n\n> That might be a little tricky, I guess.\n\nIt may be impossible to cover all cases. E.g. callers of oid_to_hex()\ncould return that buffer (like e.g. diff_aligned_abbrev()) or save\nthem in some global variable (like e.g. string_list_append() with a\nnon-DUP string_list). But safe conversion rules can got us surprisingly\nfar.\n\n> Another thing that might be tricky in this conversion is checking for\n> name conflicts with the added `hex` variable (but maybe Coccinelle\n> already has a facilitator mechanism for such cases? IDK).\n\nThat's easy. There exists no hex_buffer, yet. We can just claim that\nname for automatic conversions. It's a bit too long for people to\ntype out (we have a few variables named hexbuffer, though), but not\ncrazy long.\n\nSo what is safe? Calls of oid_to_hex() in the argument list of many\nfunctions is. E.g. puts() and printf() just consume the string that\nis passed to them, and they don't store it anywhere. That means no\nstatic storage is needed for those.\n\nstring_list_append() is only safe if it's the duplicating variant.\nSince this is a runtime option of the underlying string_list this is\nhard to prove in a Coccinelle rule. The time is better spent\nconverting them manually, I think.\n\nAnd function calls with more than four oid_to_hex() calls are broken\nalready, so we only need to have rules for up to four of them. Here\nis the simplest rule I can come up with for handling up to four\noid_to_hex() calls:\n\n@@\nidentifier fn =~ \"^(.*printf.*|puts)$\";\n@@\n(\n fn(...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer,\n ...\n ), ...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer2,\n ...\n ), ...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer3,\n ...\n ), ...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer4,\n ...\n ), ...\n )\n|\n fn(...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer,\n ...\n ), ...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer2,\n ...\n ), ...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer3,\n ...\n ), ...\n )\n|\n fn(...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer,\n ...\n ), ...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer2,\n ...\n ), ...\n )\n|\n fn(...,\n- oid_to_hex\n+ oid_to_hex_r\n (\n+ hex_buffer,\n ...\n ), ...\n )\n)\n\nSo the sub-rules are ordered from matching all four possible\noid_to_hex() calls down to a single one. Only safe consumers are\nmatched. That regex can and should be extended.\n\nHaving a list of good consumers has the nice side-effect of speeding up\nthe diff generation. It still takes a few minutes on my system, though.\n\nWe still need to declare of the local buffers. We can add them on\ndemand to each function with rules like this:\n\n@avoid_duplicates@\nidentifier buf =~ \"^hex_buffer[2-4]?$\";\n@@\n- char buf[GIT_MAX_HEXSZ + 1];\n\n@hex_buffer4_on_demand exists@\nidentifier fn;\n@@\n fn(...) {\n+ char hex_buffer4[GIT_MAX_HEXSZ + 1];\n <+... hex_buffer4 ...+>\n }\n\n@hex_buffer3_on_demand exists@\nidentifier fn;\n@@\n fn(...) {\n+ char hex_buffer3[GIT_MAX_HEXSZ + 1];\n <+... hex_buffer3 ...+>\n }\n\n@hex_buffer2_on_demand exists@\nidentifier fn;\n@@\n fn(...) {\n+ char hex_buffer2[GIT_MAX_HEXSZ + 1];\n <+... hex_buffer2 ...+>\n }\n\n@hex_buffer_on_demand exists@\nidentifier fn;\n@@\n fn(...) {\n+ char hex_buffer[GIT_MAX_HEXSZ + 1];\n <+... hex_buffer ...+>\n }\n\nWhy remove them first? To avoid duplicates when other convertible\noid_to_hex() calls are added later and the semantic patch applied\nagain.\n\nWhy declare the buffers with function scope? To avoid shadowing.\n\nWhy are the rules reversed? They add declarations at the top, so\nhex_buffer to hex_buffer4 end up being declared in that order in\nthe resulting file.\n\nWhy not use the \"fresh identifier\" feature of Coccinelle to generate\nan unused name each time? I don't know how to integrate that into\nthe 4/3/2/1 rule above without having to repeat the list of safe\nconsumers or declaring unused buffers. And reusing buffers between\nsafe consumers is fine.\n\nRené\n"},{"id":"403261","messageId":"nycvar.QRO.7.76.6.2008101613440.50@tvgsbejvaqbjf.bet","threadId":"53752","inReplyTo":"CAHd-oW6mvaANTBX5sVEi49s_Ku71-ZXqhyePtkwbCKaTK7tm4Q@mail.gmail.com","subject":"Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-10T14:15:13Z","receivedAt":"2020-08-10T14:15:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Matheus,\n\nOn Sat, 18 Jul 2020, Matheus Tavares Bernardino wrote:\n\n> On Thu, Jul 16, 2020 at 9:56 AM Johannes Schindelin\n> wrote:\n>\n> Alternatively, if using thread-local storage is still an option, I\n> think I might have solved the problems we had in the previous\n> iteration with memory leaks on Windows. I changed our\n> pthread_key_create() emulation to start using the destructor callback\n> on Windows, through the Fiber Local Storage (FLS) API. As the\n> documentation says [1] \"If no fiber switching occurs, FLS acts exactly\n> the same as thread local storage\". The advantage over TLS is that\n> FLSAlloc() does take a callback parameter.\n\nOkay, but I am still not so enthusiastic. We can fix this in a much\nsimpler way, I believe, than introducing the first thread-local storage\nuser. Let's leave TLS until the time we actually need it?\n\nCiao,\nDscho\n"}]}