You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2019/10/30 13:50:39 UTC
[kudu-CR] [compression] Use new lz4 API
lingbinlb@gmail.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14587
Change subject: [compression] Use new lz4 API
......................................................................
[compression] Use new lz4 API
In the new version (v1.9.1) of lz4, `LZ4_decompress_fast()` is deprecated
and unsafe. It is recommended to use `LZ4_decompress_safe()`
The documentation in lz4.h is as follows:
/*! LZ4_decompress_fast() : **unsafe!**
* These functions used to be faster than LZ4_decompress_safe(),
* but it has changed, and they are now slower than LZ4_decompress_safe().
* This is because LZ4_decompress_fast() doesn't know the input size,
* and therefore must progress more cautiously in the input buffer to not read beyond the end of
block.
* On top of that `LZ4_decompress_fast()` is not protected vs malformed or malicious inputs,
making it a security liability.
* As a consequence, LZ4_decompress_fast() is strongly discouraged, and deprecated.
*/
Change-Id: Ib343fda7fff1d14ee7bc551be3ac87b068504359
---
M src/kudu/util/compression/compression_codec.cc
1 file changed, 8 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/14587/1
--
To view, visit http://gerrit.cloudera.org:8080/14587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib343fda7fff1d14ee7bc551be3ac87b068504359
Gerrit-Change-Number: 14587
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
[kudu-CR] [compression] Use new lz4 API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14587 )
Change subject: [compression] Use new lz4 API
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14587/1/src/kudu/util/compression/compression_codec.cc
File src/kudu/util/compression/compression_codec.cc:
http://gerrit.cloudera.org:8080/#/c/14587/1/src/kudu/util/compression/compression_codec.cc@184
PS1, Line 184: if (n != uncompressed_length) {
Are you sure comparing against uncompressed_length makes sense? Seems like it could be OK for it not to use the entire destination buffer. The function comment for LZ4_decompress_safe says:
/*! LZ4_decompress_safe() :
compressedSize : is the exact complete size of the compressed block.
dstCapacity : is the size of destination buffer, which must be already allocated.
@return : the number of bytes decompressed into destination buffer (necessarily <= dstCapacity)
If destination buffer is not large enough, decoding will stop and output an error code (negative value).
If the source stream is detected malformed, the function will stop decoding and return a negative result.
Note : This function is protected against malicious data packets (never writes outside 'dst' buffer, nor read outside 'source' buffer).
*/
--
To view, visit http://gerrit.cloudera.org:8080/14587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib343fda7fff1d14ee7bc551be3ac87b068504359
Gerrit-Change-Number: 14587
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 30 Oct 2019 16:49:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compression] Use new lz4 API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14587 )
Change subject: [compression] Use new lz4 API
......................................................................
[compression] Use new lz4 API
In the new version (v1.9.1) of lz4, `LZ4_decompress_fast()` is deprecated
and unsafe. It is recommended to use `LZ4_decompress_safe()`
The documentation in lz4.h is as follows:
/*! LZ4_decompress_fast() : **unsafe!**
* These functions used to be faster than LZ4_decompress_safe(),
* but it has changed, and they are now slower than LZ4_decompress_safe().
* This is because LZ4_decompress_fast() doesn't know the input size,
* and therefore must progress more cautiously in the input buffer to not read beyond the end of
block.
* On top of that `LZ4_decompress_fast()` is not protected vs malformed or malicious inputs,
making it a security liability.
* As a consequence, LZ4_decompress_fast() is strongly discouraged, and deprecated.
*/
Change-Id: Ib343fda7fff1d14ee7bc551be3ac87b068504359
Reviewed-on: http://gerrit.cloudera.org:8080/14587
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/util/compression/compression_codec.cc
1 file changed, 8 insertions(+), 7 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib343fda7fff1d14ee7bc551be3ac87b068504359
Gerrit-Change-Number: 14587
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [compression] Use new lz4 API
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14587 )
Change subject: [compression] Use new lz4 API
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib343fda7fff1d14ee7bc551be3ac87b068504359
Gerrit-Change-Number: 14587
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Oct 2019 23:24:05 +0000
Gerrit-HasComments: No
[kudu-CR] [compression] Use new lz4 API
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lingbinlb@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/14587 )
Change subject: [compression] Use new lz4 API
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14587/1/src/kudu/util/compression/compression_codec.cc
File src/kudu/util/compression/compression_codec.cc:
http://gerrit.cloudera.org:8080/#/c/14587/1/src/kudu/util/compression/compression_codec.cc@184
PS1, Line 184: if (n != uncompressed_length) {
> Are you sure comparing against uncompressed_length makes sense? Seems like
Yes, the `uncompressed_length` was stored in CFile Block Header when the cfile block was compressed(in CompressedBlockBuilder::Compress()), So here, `uncompressed_length` must be exactly equal to the size after decompression.
--
To view, visit http://gerrit.cloudera.org:8080/14587
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib343fda7fff1d14ee7bc551be3ac87b068504359
Gerrit-Change-Number: 14587
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Oct 2019 03:30:39 +0000
Gerrit-HasComments: Yes