While I could reply in more details, I'll just add that if you don't feel happy with this patch, may be feel free to contribute a better patch (and for the record, I agree that passing a flag to let grpc know whether to initialize OpenSSL or not is the right thing to do and was the first solution I proposed in original mail).
It's clear that the patch is not handling your ways of using grpc. It handles mine without any threading issue/race conditions and I am happy with it. On Tue, Apr 24, 2018 at 6:30 PM, Michael Kilburn <[email protected]> wrote: > Yeah, "un-initialization functions" meant to mean "any of initialization > or uninitialization functions". I didn't communicate my point correctly. > > > > As grpc does not do any shutdown on OpenSSL, I can ensure that my > application is only cleaning it up after I am done with grpc. > > ... which is going to work swell if someone loads/unloads grpc dynamically > ;-) > > > > As for your latest remark that SSL-library-init functions are not > threadsafe, they could also be wrapped inside the callback check. > > Oh! Good thing you reminded me -- "callback check" in this pull request > isn't threadsafe. :-) I.e. this: > > if (!CRYPTO_get_locking_callback()) { > int num_locks = CRYPTO_num_locks(); > GPR_ASSERT(num_locks > 0); > g_openssl_mutexes = static_cast<gpr_mu*>( > gpr_malloc(static_cast<size_t>(num_locks) * sizeof(gpr_mu))); > for (int i = 0; i < num_locks; i++) { > gpr_mu_init(&g_openssl_mutexes[i]); > } > CRYPTO_set_locking_callback(openssl_locking_cb); > CRYPTO_set_id_callback(openssl_thread_id_cb); > } else { > gpr_log(GPR_INFO, "OpenSSL callback has already been set."); > } > > > is broken. Does it need an explanation or it is obvious? > > > > I have not done any research on them being not thread-safe so if you > have reference literature on it, that'd be good to see. > > Library init functions typically aren't threadsafe. > > > > Still as the calls are idempotent, this thread synchronization is an > easy problem. > > If you know every detail of every call -- yes, you can add synchronization > in all spots. Problem is that when you use 3rdparty lib that uses grpc > under the cover (or it uses other lib that uses grpc) -- I bet $50 it is > not going to be done correctly. > > > On Tue, Apr 24, 2018 at 8:17 PM, Arpit Baldeva <[email protected]> wrote: > >> Your original remark said - there is still a race here with other >> thread(s) un-initializing OpenSSL (*talking about un-initializing*) >> Now you are saying - that is not what I meant by my remark -- >> SSL-library-init functions are not threadsafe, afaik. (*talking about >> initializing*) >> >> My response was for the former. The original problem I had was that grpc >> would internally trounce my settings and I can not tell everyone else in my >> setting to wait for some unknown time when grpc would trounce those >> settings in some internal thread that I don't know about and only use >> OpenSSL after it. Now, it's an easier problem that I just need to make sure >> the OpenSSL is ready to use before I call grpc_init. As grpc does not do >> any shutdown on OpenSSL, I can ensure that my application is only cleaning >> it up after I am done with grpc. >> >> As for your latest remark that SSL-library-init functions are not >> threadsafe, they could also be wrapped inside the callback check. I have >> not done any research on them being not thread-safe so if you have >> reference literature on it, that'd be good to see. Still as the calls are >> idempotent, this thread synchronization is an easy problem. >> >> >> On Tue, Apr 24, 2018 at 5:45 PM, Michael Kilburn <[email protected] >> > wrote: >> >>> Well, there is no official way to fully uninitialize OpenSSL. Also, that >>> is not what I meant by my remark -- SSL-library-init functions are not >>> threadsafe, afaik. I.e. calling them from different threads at the same >>> time is unsafe. And it is not easy to guarantee that this won't happen if >>> grpc is doing it under the covers. >>> >>> On Mon, Apr 23, 2018 at 10:43 PM, Arpit Baldeva <[email protected]> >>> wrote: >>> >>>> Grpc does not un-initialize OpenSSL. If you have other thread that is >>>> un-initing it, you can easily coordinate that after grpc shutdown. The >>>> patch is only ensuring that it is not overriding application settings. >>>> While the cleanest solution is to provide an explicit flag, the solution in >>>> the patch works as long as the user knows what’s happening behind the >>>> scenes. >>>> >>>> Sent from my iPhone >>>> >>>> On Apr 23, 2018, at 8:07 PM, 'Jiangtao Li' via grpc.io < >>>> [email protected]> wrote: >>>> >>>> Initializing OpenSSL outside gRPC is not practical in many situations. >>>> Our internal expert says the race condition only exist for OpenSSL 1.0.x >>>> and 0.9.x. We are on best effort to support older versions of OpenSSL, >>>> while our main support is BoringSSL and OpenSSL 1.1. >>>> >>>> Thanks, >>>> Jiangtao >>>> >>>> >>>> On Mon, Apr 23, 2018 at 7:30 PM CM <[email protected]> wrote: >>>> >>>>> This change didn't really fix the problem -- there is still a race >>>>> here with other thread(s) un-initializing OpenSSL. Old problem, the only >>>>> correct solution is to leave OpenSSL initialization to user. >>>>> >>>>> On Monday, April 23, 2018 at 4:18:42 PM UTC-5, Jiangtao Li wrote: >>>>>> >>>>>> Good to know. Once the patch approved and merged. It will be in next >>>>>> grpc release. >>>>>> >>>>>> >>>>>> Thanks, >>>>>> Jiangtao >>>>>> >>>>>> >>>>>> On Mon, Apr 23, 2018 at 2:10 PM Arpit Baldeva <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> Thanks for the patch. It did fix the issue! >>>>>>> >>>>>>> What release version are you targeting? >>>>>>> >>>>>>> Thanks. >>>>>>> >>>>>>> On Friday, April 20, 2018 at 2:12:05 PM UTC-7, [email protected] >>>>>>> wrote: >>>>>>>> >>>>>>>> Arpit, >>>>>>>> >>>>>>>> Could you please patch https://github.com/grpc/grpc/pull/15143 and >>>>>>>> see whether it solves your problem. >>>>>>>> >>>>>>>> On Friday, April 20, 2018 at 9:30:34 AM UTC-7, Arpit Baldeva wrote: >>>>>>>>> >>>>>>>>> I am on 1.0.2k so yeah it is a problem on that version. >>>>>>>>> >>>>>>>>> I think the simplest fix is what I mentioned in last email - grpc >>>>>>>>> init_openssl implementation can check if locking callback already >>>>>>>>> exists by >>>>>>>>> calling CRYPTO_get_id_callback and if so, skip putting it's own >>>>>>>>> locks. The >>>>>>>>> application can ensure that it has the right initialization in place >>>>>>>>> before >>>>>>>>> doing anything with grpc. >>>>>>>>> >>>>>>>>> Thanks. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Thursday, April 19, 2018 at 10:58:21 AM UTC-7, Jiangtao Li >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Which version of OpenSSL are you using? Or you are using >>>>>>>>>> BoringSSL? OpenSSL 1.1 or BoringSSL does not have such problems on >>>>>>>>>> OpenSSL >>>>>>>>>> init. >>>>>>>>>> >>>>>>>>>> For OpenSSL 1.0x, it is a valid concern. Let me check what is the >>>>>>>>>> best way to resolve this issue (pass a compiler flag, environment >>>>>>>>>> variable, >>>>>>>>>> or some API changes). >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Jiangtao >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Apr 18, 2018 at 7:34 PM Arpit Baldeva <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> To explain further, a library such as grpc intializing global >>>>>>>>>>> settings of another library is not a good practice. Looking at >>>>>>>>>>> libcurl ( >>>>>>>>>>> https://curl.haxx.se/libcurl/c/curl_global_init.html) , >>>>>>>>>>> PostgreSQL(https://www.postgresql.org/docs/9.1/static/libpq- >>>>>>>>>>> ssl.html ) (and may be there are more libraries), these take an >>>>>>>>>>> initialization flag that to decide whether OpenSSL should be >>>>>>>>>>> initialized >>>>>>>>>>> internally or not. >>>>>>>>>>> >>>>>>>>>>> Actually, looking at https://stackoverflow.com/ques >>>>>>>>>>> tions/21874152/openssl-thread-safety-callback-function-regis >>>>>>>>>>> tration-with-both-direct-call-and-i , the simplest solution >>>>>>>>>>> here is for grpc to check if locking callback already exists by >>>>>>>>>>> calling >>>>>>>>>>> CRYPTO_get_id_callback and if so, skip putting it's own locks. Rest >>>>>>>>>>> of the >>>>>>>>>>> init functions in OpenSSL are idempotent and hence can be called >>>>>>>>>>> multiple >>>>>>>>>>> times. And looks like grpc is not cleaning up OpenSSL so we are >>>>>>>>>>> fine there >>>>>>>>>>> too (looks like clean up calls are getting no-op in future - >>>>>>>>>>> https://stackoverflow.com/questions/35802643/will-ignoring-t >>>>>>>>>>> o-call-openssl-evp-cleanup-result-in-serious-flaws-or-memory >>>>>>>>>>> -leak ) . >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wednesday, April 18, 2018 at 4:09:01 PM UTC-7, Arpit Baldeva >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Again, I am not sure why you are focusing on the race >>>>>>>>>>>> condition. Race condition is not the problem. Fact that grpc >>>>>>>>>>>> decides to >>>>>>>>>>>> put some global callbacks on the OpenSSL is the problem. It should >>>>>>>>>>>> not do >>>>>>>>>>>> that. Why can't this be made optional via a compile time flag or >>>>>>>>>>>> some run >>>>>>>>>>>> time initialization parameter? >>>>>>>>>>>> >>>>>>>>>>>> On Wednesday, April 18, 2018 at 3:52:47 PM UTC-7, Jiangtao Li >>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> I don't see a convenient way to do in gRPC. SSL init can be >>>>>>>>>>>>> called multiple times, as long as it is not called in the same >>>>>>>>>>>>> time. It >>>>>>>>>>>>> seems that the best way to address is adding mutex in your >>>>>>>>>>>>> application to >>>>>>>>>>>>> make sure SSL init is not called simultaneously. >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, Apr 18, 2018 at 3:28 PM Arpit Baldeva < >>>>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Yes, there are two parallel threads that do this at the same >>>>>>>>>>>>>> time. What I was noticing is that at application shutdown, my >>>>>>>>>>>>>> application >>>>>>>>>>>>>> complained about the lock I handed over to OpenSSL not being >>>>>>>>>>>>>> unlocked. >>>>>>>>>>>>>> Looking into it, I realized that during initialization, OpenSSL >>>>>>>>>>>>>> grabbed a >>>>>>>>>>>>>> lock from my callback, during this time, grpc replaced the locks >>>>>>>>>>>>>> and >>>>>>>>>>>>>> OpenSSL unlock call got re-routed. But again, this is just a >>>>>>>>>>>>>> side effect. >>>>>>>>>>>>>> The real problem is simply the fact that grpc is setting some >>>>>>>>>>>>>> global >>>>>>>>>>>>>> callbacks on OpenSSL that should best be left to the >>>>>>>>>>>>>> application. I >>>>>>>>>>>>>> understand that this is probably done to make it easy for the >>>>>>>>>>>>>> grpc >>>>>>>>>>>>>> integrators but there are applications such as mine where there >>>>>>>>>>>>>> are other >>>>>>>>>>>>>> usage of OpenSSL and I'd rather manage the global settings >>>>>>>>>>>>>> myself than rely >>>>>>>>>>>>>> on grpc to do it. >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Wednesday, April 18, 2018 at 3:05:05 PM UTC-7, >>>>>>>>>>>>>> [email protected] wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Could you describe the race condition in details? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> In gRPC ssl_transport_security, init_openssl() is only >>>>>>>>>>>>>>> called when ssl transport security is needed. In your >>>>>>>>>>>>>>> application, you are >>>>>>>>>>>>>>> calling SSL_library_init() in a separate thread in parallel? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Wednesday, April 18, 2018 at 2:46:28 PM UTC-7, Arpit >>>>>>>>>>>>>>> Baldeva wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I am using grpc-1.10.0 and it has that code. Looking at the >>>>>>>>>>>>>>>> latest master, it still has that code - >>>>>>>>>>>>>>>> https://github.com/grpc/grpc/blob/master/src/core/tsi/ssl_ >>>>>>>>>>>>>>>> transport_security.cc - see the init_openssl function. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> The problem is not just that grpc_init initialized the >>>>>>>>>>>>>>>> OpenSSL. The problem is really that grpc is initializing >>>>>>>>>>>>>>>> OpenSSL internally >>>>>>>>>>>>>>>> and it can trounce the application settings. Grpc should let >>>>>>>>>>>>>>>> application >>>>>>>>>>>>>>>> choose if they want it to handle the OpenSSL initialization or >>>>>>>>>>>>>>>> not. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Thanks. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Wednesday, April 18, 2018 at 12:25:58 PM UTC-7, >>>>>>>>>>>>>>>> [email protected] wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Hi Arpit, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> grpc_init initializes OpenSSL for a short period (~2 days) >>>>>>>>>>>>>>>>> and the code was later removed. Do you still the problem, if >>>>>>>>>>>>>>>>> you fetch the >>>>>>>>>>>>>>>>> latest master? >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Monday, April 16, 2018 at 2:22:32 PM UTC-7, Arpit >>>>>>>>>>>>>>>>> Baldeva wrote: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> I recently pinned down a sporadic race condition in my >>>>>>>>>>>>>>>>>> application due to grpc intializing OpenSSL internally. The >>>>>>>>>>>>>>>>>> problem is that >>>>>>>>>>>>>>>>>> OpenSSL has some global callbacks that grpc is trying to >>>>>>>>>>>>>>>>>> initialize on it's >>>>>>>>>>>>>>>>>> own without the authorization of the application. The >>>>>>>>>>>>>>>>>> problem is in the >>>>>>>>>>>>>>>>>> init_openssl call in ssl_transport_security.cc which >>>>>>>>>>>>>>>>>> trounces similar calls from the application. The situation >>>>>>>>>>>>>>>>>> is made >>>>>>>>>>>>>>>>>> worse(race condition) if some application threads already >>>>>>>>>>>>>>>>>> have locks >>>>>>>>>>>>>>>>>> acquired via previous calls and then grpc changes the stuff >>>>>>>>>>>>>>>>>> underneath >>>>>>>>>>>>>>>>>> before the locks are released (hence the race condition). >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> My application has usage of OpenSSL in a wider context >>>>>>>>>>>>>>>>>> than just grpc. I get the point that grpc is wrapping this >>>>>>>>>>>>>>>>>> up to make it >>>>>>>>>>>>>>>>>> easier for standing up an application with grpc but I think >>>>>>>>>>>>>>>>>> such type of >>>>>>>>>>>>>>>>>> things should be accompanied by a compile/run time option >>>>>>>>>>>>>>>>>> supplied at >>>>>>>>>>>>>>>>>> grpc_init that let's application decide the right owner. I >>>>>>>>>>>>>>>>>> am fine with the >>>>>>>>>>>>>>>>>> option defaulting to grpc initializing the OpenSSL >>>>>>>>>>>>>>>>>> internally. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Thoughts? >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Thanks. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>> You received this message because you are subscribed to a >>>>>>>>>>>>>> topic in the Google Groups "grpc.io" group. >>>>>>>>>>>>>> To unsubscribe from this topic, visit >>>>>>>>>>>>>> https://groups.google.com/d/topic/grpc-io/Z48vpXRnJaw/unsubs >>>>>>>>>>>>>> cribe. >>>>>>>>>>>>>> To unsubscribe from this group and all its topics, send an >>>>>>>>>>>>>> email to [email protected]. >>>>>>>>>>>>>> To post to this group, send email to [email protected]. >>>>>>>>>>>>>> Visit this group at https://groups.google.com/group/grpc-io. >>>>>>>>>>>>>> To view this discussion on the web visit >>>>>>>>>>>>>> https://groups.google.com/d/msgid/grpc-io/3df13436-d771-4450 >>>>>>>>>>>>>> -85c3-d2683b08e976%40googlegroups.com >>>>>>>>>>>>>> <https://groups.google.com/d/msgid/grpc-io/3df13436-d771-4450-85c3-d2683b08e976%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>>>>>>>>>>> . >>>>>>>>>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>> You received this message because you are subscribed to a topic >>>>>>>>>>> in the Google Groups "grpc.io" group. >>>>>>>>>>> To unsubscribe from this topic, visit >>>>>>>>>>> https://groups.google.com/d/topic/grpc-io/Z48vpXRnJaw/unsubs >>>>>>>>>>> cribe. >>>>>>>>>>> To unsubscribe from this group and all its topics, send an email >>>>>>>>>>> to [email protected]. >>>>>>>>>>> To post to this group, send email to [email protected]. >>>>>>>>>>> Visit this group at https://groups.google.com/group/grpc-io. >>>>>>>>>>> To view this discussion on the web visit >>>>>>>>>>> https://groups.google.com/d/msgid/grpc-io/8821745f-9b23-4e3e >>>>>>>>>>> -b4cf-b88bce513b4b%40googlegroups.com >>>>>>>>>>> <https://groups.google.com/d/msgid/grpc-io/8821745f-9b23-4e3e-b4cf-b88bce513b4b%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>>>>>>>> . >>>>>>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>>>>>> >>>>>>>>>> -- >>>>>>> You received this message because you are subscribed to a topic in >>>>>>> the Google Groups "grpc.io" group. >>>>>>> To unsubscribe from this topic, visit https://groups.google.com/d/to >>>>>>> pic/grpc-io/Z48vpXRnJaw/unsubscribe. >>>>>>> To unsubscribe from this group and all its topics, send an email to >>>>>>> [email protected]. >>>>>>> To post to this group, send email to [email protected]. >>>>>>> Visit this group at https://groups.google.com/group/grpc-io. >>>>>>> To view this discussion on the web visit >>>>>>> https://groups.google.com/d/msgid/grpc-io/d178eff0-8225-4a0b >>>>>>> -b63f-ac15afe425e3%40googlegroups.com >>>>>>> <https://groups.google.com/d/msgid/grpc-io/d178eff0-8225-4a0b-b63f-ac15afe425e3%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>>>> . >>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>> >>>>>> -- >>>>> You received this message because you are subscribed to a topic in the >>>>> Google Groups "grpc.io" group. >>>>> To unsubscribe from this topic, visit https://groups.google.com/d/to >>>>> pic/grpc-io/Z48vpXRnJaw/unsubscribe. >>>>> To unsubscribe from this group and all its topics, send an email to >>>>> [email protected]. >>>>> To post to this group, send email to [email protected]. >>>>> Visit this group at https://groups.google.com/group/grpc-io. >>>>> To view this discussion on the web visit >>>>> https://groups.google.com/d/msgid/grpc-io/221ffdb6-aa2c-457d >>>>> -8772-32ace46173dc%40googlegroups.com >>>>> <https://groups.google.com/d/msgid/grpc-io/221ffdb6-aa2c-457d-8772-32ace46173dc%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>> . >>>>> For more options, visit https://groups.google.com/d/optout. >>>>> >>>> -- >>>> You received this message because you are subscribed to a topic in the >>>> Google Groups "grpc.io" group. >>>> To unsubscribe from this topic, visit https://groups.google.com/d/to >>>> pic/grpc-io/Z48vpXRnJaw/unsubscribe. >>>> To unsubscribe from this group and all its topics, send an email to >>>> [email protected]. >>>> To post to this group, send email to [email protected]. >>>> Visit this group at https://groups.google.com/group/grpc-io. >>>> To view this discussion on the web visit https://groups.google.com/d/ms >>>> gid/grpc-io/CACcm8_i7f6HdGG3G72p_FN2UqwyUzaDyDVKGE3oNP1Xbi9G >>>> SwA%40mail.gmail.com >>>> <https://groups.google.com/d/msgid/grpc-io/CACcm8_i7f6HdGG3G72p_FN2UqwyUzaDyDVKGE3oNP1Xbi9GSwA%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>> . >>>> For more options, visit https://groups.google.com/d/optout. >>>> >>>> >>> >>> >>> -- >>> Sincerely yours, >>> Michael. >>> >> >> > > > -- > Sincerely yours, > Michael. > -- You received this message because you are subscribed to the Google Groups "grpc.io" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at https://groups.google.com/group/grpc-io. To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/CAL2p%3D0kwXPEVQ-tzDsDtSG--1YQOcQ7FNP%2BOdp6uobT3kiw30w%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
