Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5221: Avoid re-use of stale SASL contexts. ......................................................................
Patch Set 2: (14 comments) http://gerrit.cloudera.org:8080/#/c/7116/1//COMMIT_MSG Commit Message: PS1, Line 7: Avoid re-use of stale SASL contexts. > specifically: don't re-use old sasl contexts, right? Yup, done. http://gerrit.cloudera.org:8080/#/c/7116/1/be/src/transport/TSasl.cpp File be/src/transport/TSasl.cpp: PS1, Line 119: } : /* : if (!authenticationId.empty()) { : // TODO: setup security property : sasl_security_properties_t secprops; : // populate secprop > move these initializations to the member initializer list (e.g. service(ser Whoops, thanks. Done. PS1, Line 128: : > int result = ... Done Line 129: > IIUC, we should DCHECK(conn == nullptr) here to confirm we're not hitting t Done PS1, Line 208: const string& userRealm, unsigned flags, sasl_callback_t* callbacks) : : TSasl(service, serverFQDN, callbacks), : userRealm(userRealm), : flags(flags), : serverStarted(false) { } : : void TSaslServer::setupS > move to member initializer list etc. Done PS1, Line 218: NULL, NULL, callbacks, flags, &conn); : if (re > int result = ... Done http://gerrit.cloudera.org:8080/#/c/7116/1/be/src/transport/TSasl.h File be/src/transport/TSasl.h: PS1, Line 61: setupSaslContext > I think setupSaslContext is a better name. Done PS1, Line 66: dispose > can't this lead to calling sasl_dispose(&nullptr) if there's a negotiation Yes, I completely missed that. I've added a null check in dispose(). It looks like the code would return a bad status if the pointer it receives is invalid and we ignore it anyway because it's the destructor. That probably explains why we've never seen it crash here with my patch. PS1, Line 69: * Call > why is this one virtual? and how about disposeSaslContext()? Done Line 71: * with servers or done with clients. Internally the library maintains a which > doesn't this need to be wrapped in "if (conn != nullptr)"? i.e. can't we ge Yes, I added the check here, it could end up here with conn==nullptr with the case Henry mentioned. It looks like the code would return a bad status if the pointer it receives is invalid and we ignore it anyway because it's the destructor. That probably explains why we've never seen it crash here with my patch. PS1, Line 69: * Called once per application to free resources.` : * Note that there is no distinction in the sasl library between being done : * with servers or done with clients. Internally the library maintains a which : > can this be protected? Done Line 131: sasl_conn_t* conn; > who owns it? and any way to be more descriptive than "registered callbacks" Done http://gerrit.cloudera.org:8080/#/c/7116/1/be/src/transport/TSaslClientTransport.h File be/src/transport/TSaslClientTransport.h: PS1, Line 51: Set u > Set up Done http://gerrit.cloudera.org:8080/#/c/7116/1/be/src/transport/TSaslServerTransport.h File be/src/transport/TSaslServerTransport.h: PS1, Line 44: /* Set up the Sasl server state for a connection. > If this is a no-op, the comment shold be here. Done -- To view, visit http://gerrit.cloudera.org:8080/7116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7d509e14dfe46fa28d0626cf84daf6de82955d80 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-HasComments: Yes
