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.

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

background fetching can cause data corruption #842

Closed
vadimberezniker opened this issue Jul 7, 2022 · 3 comments · Fixed by #843
Closed

background fetching can cause data corruption #842

vadimberezniker opened this issue Jul 7, 2022 · 3 comments · Fixed by #843
Labels
bug Something isn't working

Comments

@vadimberezniker
Copy link
Contributor

vadimberezniker commented Jul 7, 2022

I have been experimenting with container image streaming using stargz-store + podman, but have run into some odd behavior which I have narrowed down to the "background fetch" feature. When background fetching is enabled, the store can return wrong contents for files.

After a few days of debugging, I think the following is happening:
When a layer is first resolved, a new memory.reader is created. memory.NewReader assigns an ID to each file.
It's important to note that the ID assignment is not deterministic because ForEachChild internally iterates over a map which does not provide a stable ordering. The assigned ID is used to compute the key used for storing data in the cache.

Background fetching calls verifiableReader.Cache(reader.WithReader(...)) which calls Clone on metadata.Reader. In the memory.reader case, the IDs are not preserved which means the newly assigned IDs may not match the original assignments. Background fetching can then populate the cache with chunks for the "wrong" IDs.

I think a simple fix is to copy over the ID assignments when Clone is called, what do you think?

@ktock
Copy link
Member

ktock commented Jul 7, 2022

@vadimberezniker Thank you for the detailed report.

I think a simple fix is to copy over the ID assignments when Clone is called, what do you think?

Yes, that solution sounds good to me. Would you be willing to create a patch to fix it?

@ktock ktock added the bug Something isn't working label Jul 7, 2022
@vadimberezniker
Copy link
Contributor Author

Yes, I can work on a patch tomorrow.

@ktock
Copy link
Member

ktock commented Jul 7, 2022

@vadimberezniker Thanks!

vadimberezniker added a commit to vadimberezniker/stargz-snapshotter that referenced this issue Jul 7, 2022
As the IDs are used for computing cache keys, it's important that they
do not change. Prior to this change, background fetching would clone the
reader and populate the cache with entries using possibly incorrect keys.

This patch changes the clone behavior to copy over the original ID
mappings.

Note that I also removed the locks around the ID maps as these maps are
never updated after the reader is created.

Fixes containerd#842
vadimberezniker added a commit to vadimberezniker/stargz-snapshotter that referenced this issue Jul 8, 2022
As the IDs are used for computing cache keys, it's important that they
do not change. Prior to this change, background fetching would clone the
reader and populate the cache with entries using possibly incorrect keys.

This patch changes the clone behavior to copy over the original ID
mappings.

Note that I also removed the locks around the ID maps as these maps are
never updated after the reader is created.

Fixes containerd#842

Signed-off-by: Vadim Berezniker <vadim@berezniker.com>
@ktock ktock closed this as completed in #843 Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants