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

Bug fix for segfault #105

Merged
merged 2 commits into from Sep 13, 2023
Merged

Bug fix for segfault #105

merged 2 commits into from Sep 13, 2023

Conversation

usefulcat
Copy link
Contributor

In read_thread(), in the "Do we need this block?" block, it failed
to advance to the next wanted_t when w->end is exactly equal to uend.

In the particular case I looked at, this resulted in read_thread()
erroneously putting two blocks into the queue (pipeline_split(),
read.c:570) instead of one.

This resulted in a subsequent crash in tar_read() at read.c:660,
where gArWanted was null (when clearly the code does not expect it
to be null at that point).

My use case:

I have several hundred large .tar.xz files (created by pixz). Each
archive contains over 200k files. I frequently need to extract a lot
of files from each archive, but not all files, only a specific subset.
So I am making heavy use of the -x option to pixz.

In read_thread(), in the "Do we need this block?" block, it failed
to advance to the next wanted_t when w->end is exactly equal to uend.

In the particular case I looked at, this resulted in read_thread()
erroneously putting two blocks into the queue (pipeline_split(),
read.c:570) instead of one.

This resulted in a subsequent crash in tar_read() at read.c:660,
where gArWanted was null (when clearly the code does not expect it
to be null at that point).

My use case:

I have several hundred large .tar.xz files (created by pixz). Each
archive contains over 200k files. I frequently need to extract a lot
of files from each archive, but not all files, only a specific subset.
So I am making heavy use of the -x option to pixz.
Don't hold queue mutex while calling malloc/free
@usefulcat
Copy link
Contributor Author

Update: I've been using this fix for over 8 months so far with no problems.

@@ -536,7 +536,7 @@ static void read_thread(void) {
debug("read: skip %llu", iter.block.number_in_file);
continue;
}
for ( ; w && w->end < uend; w = w->next) ;
for ( ; w && w->end <= uend; w = w->next) ;
Copy link

@xkszltl xkszltl Sep 13, 2023

Choose a reason for hiding this comment

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

Does it make sense to have <= here together with the >= in the if condition above?

Copy link

Choose a reason for hiding this comment

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

nvm that's start, I think this makes sense.

@xkszltl
Copy link

xkszltl commented Sep 13, 2023

We run into the same issue on Ubuntu 22.04 with its stock pixz 1.0.7, where pixz -x path -i file.txz segfault after printing out most of the file content.
Did some debugging with gdb and found the same root cause, i.e. gArWanted = 0x0 in tar_read().
This bug would be triggered whenever a file end aligns with block boundary.

It's also unsafe to assume, in tar_read(), every block has at least one file from gArWanted to pair with.
Better to early return as well.

@vasi
Could you take a look?
I think this PR should be merged.

@xkszltl
Copy link

xkszltl commented Sep 13, 2023

We also confirmed the crash on that particular file can be mitigated by:

  • Add -f 3.0 to move the block boundary
  • Append another -x next_path to include more blocks.

@vasi vasi merged commit 8155add into vasi:master Sep 13, 2023
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.

None yet

3 participants