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

Fix #46 and #42 and #50 #51

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

solsticedhiver
Copy link
Contributor

@solsticedhiver solsticedhiver commented Nov 8, 2021

Add all the code and test to deal with NoProgressBar option in /etc/pacman.conf

and fixes man pages issues
and fixes armv7h issues.

@solsticedhiver solsticedhiver changed the title Fix #46 Fix #46 and #42 Nov 8, 2021
Fix segfault on 32 bits architecture (like armv6h, armv7h) by forcing a 64 bits size for off_t
by using _FILE_OFFSET_BITS=64
@solsticedhiver solsticedhiver changed the title Fix #46 and #42 Fix #46 and #42 and #50 Nov 8, 2021
@Nils-TUD
Copy link

I stumbled upon the same issue on my ARMv7 system and fortunately found your fix :)

However, there is another bug if the argument --file-properties is used. It segfaults somewhere in sprintf. I tracked that down and found that the problem is in the pu_hr_size function, which is called in cmp_size in paccheck.c:523. Unfortunately, the behavior is really weird and therefore I don't know how to fix it yet.

The problem is again related to the off_t type, which is the type of the first parameter of pu_hr_size. Changing the type to, e.g., int makes the problem disappear. I also noticed that it is only the argument transfer that causes the problem. Using int for the parameter and assigning that to a local variable of type off_t also makes the problem disappear.

What is even more weird is that if I copy the function pu_hr_size, which resides in libpacutils.so, to a local function in paccheck.c (without any change), everything works. But the util.c from libpacutils.so and paccheck.c are compiled with the same flags (except for necessary differences like -fPIC and -shared for the util.c). The resulting assembly code of both functions is identical. The code for the function call is also identical, no matter whether the local or library function is called.

So, it seems like transferring 64-bit arguments across a library boundary causes the problem on 32-bit ARM. But that doesn't make any sense to me. Any ideas?

@solsticedhiver
Copy link
Contributor Author

solsticedhiver commented Nov 20, 2021

Using my fork, with the patches, I don't get any errors when using --file-properties, on armv7h. (edit) I am using pacutils-git, with a modified PKGBUILD pointing at my repo fork.
Do you use my fork, or the original paccheck ?

In whatever case, what command are you running ?

After a quick look at cmp_size, I just noticed that psize is declared as an int64_t, but pu_hr_size expects an off_t. If off_t is not 64bits, that could cause some problem ?

Beside that, I don't know what to tell you, because I am not a super expert in C.

@Nils-TUD
Copy link

Using my fork, with the patches, I don't get any errors when using --file-properties, on armv7h. (edit) I am using pacutils-git, with a modified PKGBUILD pointing at my repo fork. Do you use my fork, or the original paccheck ?

In whatever case, what command are you running ?

I'm using your fork. Now that you ask I noticed that this segfault only occurs if you have a file in your system where the size doesn't match. Maybe that doesn't happen for you? Because I think the error always occurs if the function pu_hr_size in line 539 is called. At least I don't see anything specific on my side (the size passed to pu_hr_size is 45 kB).

After a quick look at cmp_size, I just noticed that psize is declared as an int64_t, but pu_hr_size expects an off_t. If off_t is not 64bits, that could cause some problem ?

No, that's fine and off_t is indeed 64-bit now that _FILE_OFFSET_BITS=64 is defined by your patch. However, I don't think your patch is wrong. There should be a way to solve this problem.

@solsticedhiver
Copy link
Contributor Author

I don't see any error or segfault either when a file size does not match.

I did not find by myself that solution. I was asking pacman's dev, about a problem, when simply listing files of a package. This is where the segfault happens for paccheck. So I was given that quick fix (using _FILE_OFFSET_BITS)

A demo code showing the problem: https://paste.rs/5Os

@Nils-TUD
Copy link

I don't see any error or segfault either when a file size does not match.

hm, that's strange. No idea why that happens for me then.

I did not find by myself that solution. I was asking pacman's dev, about a problem, when simply listing files of a package. This is where the segfault happens for paccheck. So I was given that quick fix (using _FILE_OFFSET_BITS)

A demo code showing the problem: https://paste.rs/5Os

Ok. I also checked that again. Your patch is indeed correct. The problem is in check_files where it iterates over the files found by alpm_pkg_get_files. To do that, it indexes into the array in alpm_filelist_t, whose elements have the type alpm_file_t. And this type contains an off_t. Thus, off_t needs to have the same width for both the alpm library and the application linking against that. Previously this wasn't true for paccheck on 32-bit ARM and that's why it segfaulted. So, making sure that off_t is 64-bit as also mentioned in a commit in pacman is correct as far as I can see.

The question is only how the problem that it introduces can be fixed...

@Nils-TUD
Copy link

Oh, it was my fault. I didn't see that it still used the old libpacutils.so instead of the one I built 🙈

Everything works fine with your patch. Sorry for the confusion.

@solsticedhiver
Copy link
Contributor Author

solsticedhiver commented Nov 20, 2021

oh. You got bitten with that problem too. 😁 I was too when trying to run my modified version....

I would suggest you to use pacutils-git PKGBUILD but change the repo to use mine. my 2 cents..

@Nils-TUD
Copy link

Yeah, it's pretty difficult to try it out without installing it apparently 😁 Taking the PKGBUILD from pacutils-git and changing the repo sounds good.

@solsticedhiver
Copy link
Contributor Author

solsticedhiver commented Nov 20, 2021

I was doing LD_LIBRARY_PATH=./lib ./src/pacheck during the dev time if I remember correctly

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