Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18472 )
Change subject: jwt: determine discovery endpoint from token ...................................................................... Patch Set 14: (11 comments) http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc@1086 PS12, Line 1086: > Isn't a race to update the 'subject' from different threads when they apply Ack http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc@1089 PS12, Line 1089: O > Does it make sense to add an assertion on the expected result status of the Done http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc@1091 PS12, Line 1091: }; > +1: return isn't needed there Ack http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util-test.cc@1094 PS12, Line 1094: std::vector<std::thread> threads; : for (int i = 0; i < N; i++) { > Could these be placed in a container? Also, having more threads than just Done http://gerrit.cloudera.org:8080/#/c/18472/10/src/kudu/util/jwt-util.h File src/kudu/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/18472/10/src/kudu/util/jwt-util.h@120 PS10, Line 120: explicit PerAccountKeyBasedJwtVerifier(std::string jwks_uri) > nit: add a blank line after each declaration for readability Done http://gerrit.cloudera.org:8080/#/c/18472/10/src/kudu/util/jwt-util.h@131 PS10, Line 131: // Returns an error if the token doesn't contain the appropriate fields. > nit: capitalize the first letter and add a period at the end of the sentenc Done http://gerrit.cloudera.org:8080/#/c/18472/10/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18472/10/src/kudu/util/jwt-util.cc@952 PS10, Line 952: } > nit: add a blank line after each closing curly brace Done http://gerrit.cloudera.org:8080/#/c/18472/10/src/kudu/util/jwt-util.cc@960 PS10, Line 960: const auto* unique_helper = FindOrNull(jwt_by_account_id_, account_id); > nit: add a blank line above this Done http://gerrit.cloudera.org:8080/#/c/18472/10/src/kudu/util/jwt-util.cc@982 PS10, Line 982: dst.push_back('\0'); > Should this be #undef-ed when it's not used anymore? Done http://gerrit.cloudera.org:8080/#/c/18472/10/src/kudu/util/jwt-util.cc@994 PS10, Line 994: RETURN_INVALID_IF(!jwks_uri_member->value.IsString(), "jwks_uri must be a string"); > nit: maybe change the todo to yours? Done http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18472/12/src/kudu/util/jwt-util.cc@997 PS12, Line 997: > I'd think moving this to be after initting the newly created helper, to gua This was moved here as the ASAN test throws an error as follows: SUMMARY: AddressSanitizer: heap-use-after-free ../src/kudu/util/jwt-util.cc:826:3 in kudu::JWTHelper::Verify(kudu::JWTHelper::JWTDecodedToken const*) const So I thought there was some race there as well, however it seems that I was wrong and the issue is still present. -- To view, visit http://gerrit.cloudera.org:8080/18472 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I970bcc6d894c0206160196418d549b71c35ac973 Gerrit-Change-Number: 18472 Gerrit-PatchSet: 14 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 20 Jan 2023 17:38:42 +0000 Gerrit-HasComments: Yes
