Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

API: add LZ4_decompress_safe_partial_usingDict to support partial decompression with dict #1093

Merged
merged 4 commits into from Jun 12, 2022
Merged

API: add LZ4_decompress_safe_partial_usingDict to support partial decompression with dict #1093

merged 4 commits into from Jun 12, 2022

Conversation

yawqi
Copy link
Contributor

@yawqi yawqi commented Jun 7, 2022

Add LZ4_decompress_safe_partial_usingDict API to support partial decompression with dictionary.

This patch is still working in progress, I will appreciate it if you guys could give me some suggestions. Thanks for your review!

Feature request #1051.

@yawqi
docs: fix some typo
51be294 
Signed-off-by: Qi Wang <wangqi@linux.alibaba.com>
@yawqi yawqi force-pushed the partial-with-dict branch 3 times, most recently from dacd14c to 8540b21 Compare June 7, 2022 04:08
Cyan4973
Cyan4973 reviewed Jun 7, 2022
View reviewed changes
lib/lz4.h Outdated
@@ -777,6 +778,8 @@ LZ4LIB_API int LZ4_decompress_fast_usingDict (const char* src, char* dst, int or
LZ4LIB_API void LZ4_resetStream (LZ4_stream_t* streamPtr);


/*! Obsolete partial decompress with dict functions */
LZ4_DEPRECATED("use LZ4_decompress_safe_partial_usingDict() instead") LZ4LIB_API int LZ4_decompress_safe_partial_withPrefix64k(const char* source, char* dest, int compressedSize, int targetOutputSize, int dstCapacity);
Copy link
Member

@Cyan4973 Cyan4973 Jun 7, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unusual for a newly introduced function to be immediately classified "deprecated".
Is this entry added for the sake of consistency with LZ4_decompress_safe_withPrefix64k() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review! Yes, should I remove it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done.

Cyan4973
Cyan4973 reviewed Jun 7, 2022
View reviewed changes
ossfuzz/round_trip_fuzzer.c
@@ -47,7 +52,61 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
FUZZ_ASSERT_MSG(!memcmp(data, partial, partialSize), "Corruption!");
free(partial);
}

/* Partial decompression using dict with no dict. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty nice,
good testing is a must for a new feature.

@Cyan4973
Copy link
Member

Cyan4973 commented Jun 7, 2022 •
edited

The implementation looks fine to me.
I believe it "just" re-employs internal functions within a scope which is "more complete".
So that limits amount of changes, hence of risks, in the engine.

Moreover, it comes with a great battery of tests, which is a welcome relief for reliability concerns.

I'm a little concerned by the length of the selected name LZ4_decompress_safe_partial_usingDict(), which sounds like a mouthful, but have no better counter-proposal to suggest. Plus, it respects the existing naming convention, so it doesn't feel "out of place".

@Cyan4973
Copy link
Member

Cyan4973 commented Jun 7, 2022 •
edited

OK, I'll go ahead and say that it's almost completely fine, on first attempt. So that's great job. Even the name is green-lighted, because it's a direct reference to existing LZ4_decompress_safe_partial.

My only comment is about LZ4_decompress_safe_partial_withPrefix64k().
I don't think it's worthwhile to publish this entry and make it immediately "deprecated".
The reason we have deprecated entries in lz4.h is because they used to be part of the official API, therefore some existing programs might still be depending on them, and we don't want to break their linkage just for a version update.
However, for this specific instance, there is no prior usage of this entry point since it never existed.
So no need to publish it at all.
This is one less entry point to maintain, and also one less potential vulnerability vector to worry about in the future.

@yawqi
Copy link
Contributor Author

yawqi commented Jun 7, 2022

OK, I'll go ahead and say that it's almost completely fine, on first attempt. So that's great job. Even the name is green-lighted, because it's a direct reference to existing LZ4_decompress_safe_partial.

My only comment is about LZ4_decompress_safe_partial_withPrefix64k(). I don't think it's worthwhile to publish this entry and make it immediately "deprecated". The reason we have deprecated entries in lz4.h is because they used to be part of the official API, therefore some existing programs might still be depending on them, and we don't want to break their linkage just for a version update. However, for this specific instance, there is no prior usage of this entry point since it never existed. So no need to publish it at all. This is one less entry point to maintain, and also one less potential vulnerability vector to worry about in the future.

Thanks for your review! I will remove the LZ4_decompress_safe_partial_withPrefix64k entry.

yawqi added 3 commits June 7, 2022 17:12
@yawqi
API: add LZ4_decompress_safe_partial_usingDict API
b1e84b3 
feature request: lz4#1051

Signed-off-by: Qi Wang <wangqi@linux.alibaba.com>
@yawqi
tests: add tests for LZ4_decompress_safe_partial_usingDict
ec75db2 
Signed-off-by: Qi Wang <wangqi@linux.alibaba.com>
@yawqi
ossfuzz: add fuzz for LZ4_decompress_safe_partial_usingDict
582f5fe 
Signed-off-by: Qi Wang <wangqi@linux.alibaba.com>
@yawqi yawqi changed the title WIP: API, add LZ4_decompress_safe_partial_usingDict to support partial decompression with dict API: add LZ4_decompress_safe_partial_usingDict to support partial decompression with dict Jun 8, 2022
@hsiangkao
Copy link

Hi Cyan, could we merge it so I can develop more based on this? Many thanks!

@Cyan4973 Cyan4973 merged commit 4ebe313 into lz4:dev Jun 12, 2022
@hsiangkao
Copy link

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cyan4973 Cyan4973 Cyan4973 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yawqi @Cyan4973 @hsiangkao