[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-12418: Include crypto functions supported by Hive ...................................................................... Patch Set 8: (10 comments) > Uploaded patch set 8. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1826 PS6, Line 1826: static int encrypt_ecb(FunctionContext* ctx, const unsigned char* input, int input_len, > This function doesn't exist in OpenSSL 1.0.2. It appears it was added in 1. I don't think we can do that as all of the functions that follow uses an uninitialized EVP_CIPHER_CTX. Doing this is leading to such errors: :1835:25: error: ‘cipher_ctx’ is used uninitialized in this function [-Werror=uninitialized] 1835 | if(!EVP_EncryptInit_ex(cipher_ctx, cipher, nullptr, key, nullptr)) { | http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1842 PS6, Line 1842: } > Let's add a DCHECK that len >= input_len and len <= the length we require b Done http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1865 PS6, Line 1865: EVP_CIPHER_CTX_free(cipher_ctx); > Please document what length we require allocated for output here. Added at 1905 http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1867 PS6, Line 1867: return cipher_len; > See comment at line 1826. Ack http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1913 PS6, Line 1913: return cipher_len; > By "this", I meant that block size is 16 bytes. Or at least include a DCHEC Done http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1938 PS6, Line 1938: cipher = EVP_aes_256_ecb(); > If we encounter this, won't we have already encountered an error and SetErr I think this is the only place where we're checking for output_len. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1952 PS6, Line 1952: > Let's use EVP_CIPHER_block_size to determine this. Done http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1971 PS6, Line 1971: case 24: > I don't think it makes sense to call CopyFrom(ctx, output_vec, -1); Done http://gerrit.cloudera.org:8080/#/c/20447/7/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/7/be/src/exprs/string-functions-ir.cc@1826 PS7, Line 1826: static int encrypt_ecb(FunctionContext* ctx, const unsigned char* input, int input_len, > These are internal, lets make them 'static' functions. Done http://gerrit.cloudera.org:8080/#/c/20447/7/be/src/exprs/string-functions-ir.cc@1862 PS7, Line 1862: DCHECK(EVP_CIPHER_block_size(cipher) == 16); > I don't think this check is correct. len is whatever EVP_EncryptFinal_ex ju Done -- 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: 8 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[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-Comment-Date: Tue, 23 Jan 2024 18:34:18 +0000 Gerrit-HasComments: Yes
