impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5221: Avoid re-use of stale SASL contexts.
Date Fri, 09 Jun 2017 01:18:49 GMT
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 <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message