Skip to content

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

Merged
Cyan4973 merged 4 commits intolz4:devfrom
yawqi:partial-with-dict
Jun 12, 2022
Merged

API: add LZ4_decompress_safe_partial_usingDict to support partial decompression with dict#1093
Cyan4973 merged 4 commits intolz4:devfrom
yawqi:partial-with-dict

Conversation

@yawqi
Copy link
Copy Markdown
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 <[email protected]>
@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
Comment thread lib/lz4.h Outdated


/*! 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
Copy Markdown
Member

@Cyan4973 Cyan4973 Jun 7, 2022 •
edited
Loading

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
Copy Markdown
Contributor Author

@yawqi yawqi Jun 7, 2022

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
Copy Markdown
Member

@Cyan4973 Cyan4973 Jun 7, 2022

Choose a reason for hiding this comment

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

Yes please

Copy link
Copy Markdown
Contributor Author

@yawqi yawqi Jun 7, 2022

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
Comment thread ossfuzz/round_trip_fuzzer.c
free(partial);
}

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

@Cyan4973 Cyan4973 Jun 7, 2022

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
Copy Markdown
Member

Cyan4973 commented Jun 7, 2022 •
edited
Loading

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
Copy Markdown
Member

Cyan4973 commented Jun 7, 2022 •
edited
Loading

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
Copy Markdown
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 yawqi force-pushed the partial-with-dict branch from 8540b21 to 582f5fe Compare June 7, 2022 09:13
@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
Copy Markdown

hsiangkao commented Jun 12, 2022

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
Copy Markdown

hsiangkao commented Jun 13, 2022

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.

3 participants

@yawqi @Cyan4973 @hsiangkao