Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 21: (5 comments) http://gerrit.cloudera.org:8080/#/c/20447/21/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/21/be/src/exprs/string-functions-ir.cc@1875 PS21, Line 1875: S Nit: add space after the comma. http://gerrit.cloudera.org:8080/#/c/20447/21/be/src/exprs/string-functions-ir.cc@1875 PS21, Line 1875: $0 Seeing the test with the empty string, it's better to surround the mode with quotes (either single or double), otherwise it looks strange with an empty string. Applies also to aes_encrypt(). http://gerrit.cloudera.org:8080/#/c/20447/21/be/src/exprs/string-functions-ir.cc@1884 PS21, Line 1884: encryption_key ModeToString() is static, should call it like EncryptionKey::ModeToString() as in previous patch sets. Applies also to 1968. http://gerrit.cloudera.org:8080/#/c/20447/21/be/src/exprs/string-functions-ir.cc@1906 PS21, Line 1906: AES_BLOCK_SIZE In case of decryption, the clear text size should never be greater than the ciphertext size. When the two don't match is in the case of ECB, when a padding block was necessary during encryption. In this case the clear text is one block shorter than the ciphertext, so allocating 'len' is always enough. http://gerrit.cloudera.org:8080/#/c/20447/21/be/src/exprs/string-functions-ir.cc@1915 PS21, Line 1915: result.len = out_len; Before this assignment, we could add this to assert the correctness of the sizes: const int64_t len_diff = result.len - out_len; DCHECK(len_diff == 0 || (encryption_key.IsEcbMode() && 0 < len_diff && len_diff <= AES_BLOCK_SIZE)); -- To view, visit http://gerrit.cloudera.org:8080/20447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3902f2b1d95da4d06995cbd687e79c48e16190c9 Gerrit-Change-Number: 20447 Gerrit-PatchSet: 21 Gerrit-Owner: Pranav Lodha <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Comment-Date: Tue, 08 Oct 2024 10:44:37 +0000 Gerrit-HasComments: Yes
