Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17251 )
Change subject: [encryption] Add encryption support to Env ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@224 PS7, Line 224: // TODO(abukor): delete this Do we want to file a bug to track this which is linked to parent/epic JIRA to make sure it's indeed removed? http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@380 PS7, Line 380: its Nit: it's http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@397 PS7, Line 397: unsigned char scratch_clear[kEncryptionBlockSize]; : unsigned char scratch_cipher[kEncryptionBlockSize]; : I'm not familiar with the OpenSSL encryption APIs but should these be zero initialized? unsigned char scratch_clear[kEncryptionBlockSize] = {0}; http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@579 PS7, Line 579: idx I got a bit confused between idx and i below. Could you consider renaming one of the variable names. http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@586 PS7, Line 586: RETURN_NOT_OK(DoEncryptV(kDummyEncryptionKey, offset, data, encrypted_data)); > What happens with the memory allocated for 'encrypted_buf' when DoEncryptV( Perhaps the scoped_cleanup below needs to move above around L579. Can even consider using unique_ptr for encrypted_buf. -- To view, visit http://gerrit.cloudera.org:8080/17251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5a81b5736a0c1970818dbbc4a234480476b8d41 Gerrit-Change-Number: 17251 Gerrit-PatchSet: 7 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 21 Apr 2021 19:20:56 +0000 Gerrit-HasComments: Yes
