Skip to content

fix panic when chunk size exceeds merge buffer size #2068

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.

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

Merged
merged 1 commit into from
Jun 27, 2025

Conversation

wswsmao
Copy link
Contributor

@wswsmao wswsmao commented Jun 25, 2025

Add a check to ensure that chunkSize does not exceed mergeBufferSize.
If chunkSize is greater than mergeBufferSize, return an error to prompt the user to update the config or adjust the image chunk size.
This prevents potential slice bounds out of range panics during merging.

@wswsmao wswsmao closed this Jun 25, 2025
@wswsmao wswsmao reopened this Jun 25, 2025
// Check if chunk size exceeds merge buffer size to prevent slice bounds out of range panic
// when mergeBufferSize is smaller than chunkSize, report error to suggest user to
// update config or adjust image chunk size
if chunkSize > mergeBufferSize {
Copy link
Member

Choose a reason for hiding this comment

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

why not dynamically allocate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dynamic allocation also carries risks. For example, in environments with limited memory, if the chunk size is large and multiple chunks are being downloaded concurrently, it could lead to OOM (out-of-memory) issues—similar to the problem addressed in this PR.

The purpose of making mergeBufferSize configurable is to allow users to set an acceptable buffer size based on their own environment’s resources and the characteristics of their images.

Copy link
Member

Choose a reason for hiding this comment

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

allow users to set an acceptable buffer size based on their own environment’s resources and the characteristics of their images.

It's better to automatically control (reduce or disable) the concurrency not to exceed the buffer limit. Snapshotter should not fail depending on the image size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, when chunkSize > mergeBufferSize, it will automatically fall back to sequential processing using the legacy approach.

@wswsmao wswsmao force-pushed the main branch 2 times, most recently from 498ed55 to df76ff2 Compare June 26, 2025 02:43
Copy link
Member

@ktock ktock left a comment

Choose a reason for hiding this comment

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

Thanks

@ktock ktock merged commit 5a1b37f into containerd:main Jun 27, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants