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] > <javascript:>> 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/questions/21874152/openssl-thread-safety-callback-function-registration-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-to-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/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/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/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/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/topic/grpc-io/Z48vpXRnJaw/unsubscribe. >> To unsubscribe from this group and all its topics, send an email to >> [email protected] <javascript:>. >> To post to this group, send email to [email protected] >> <javascript:>. >> 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 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/221ffdb6-aa2c-457d-8772-32ace46173dc%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
