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 compilation on 32bit architectures such as i386, armhf #34

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

siretart
Copy link

@siretart siretart commented Oct 20, 2024

On those architectures, the tests fail to build with an overflow error:

src/github.com/cyphar/filepath-securejoin/mkdir_linux_test.go:254:8: constant 2147484159 overflows int FAIL
github.com/cyphar/filepath-securejoin [build failed]

Turns out that FileMode is defined as an uint32 in the os.fs package. Since MkdirAll is intended as a replacement for os.MkdirAll, it makes sense to use the same types for all arguments, and that means os.FileMode for the mode parameter.

On those architectures, the tests fail to build with an overflow error:

src/github.com/cyphar/filepath-securejoin/mkdir_linux_test.go:254:8: constant 2147484159 overflows int
FAIL	github.com/cyphar/filepath-securejoin [build failed]

Turns out that FileMode is defined as an uint32 in the os.fs package.
Since `MkdirAll` is intended as a replacement for os.MkdirAll, it makes
sense to use the same types for all arguments, and that means os.FileMode
for the mode parameter.

Signed-off-by: Reinhard Tartler <[email protected]>
@@ -22,6 +22,8 @@ var (
errPossibleAttack = errors.New("possible attack detected")
)

type FileMode = os.FileMode
Copy link
Owner

@cyphar cyphar Dec 6, 2024

Choose a reason for hiding this comment

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

The problem with doing this (and this is the reason why I didn't use os.FileMode in the first place) is that the os.FileMode bits don't match the Unix bits outside of 0o777. Passing the wrong mode bits is a common bug and accepting os.FileMode would lead to confusion unless we also added conversion code as well. (To be fair, we would return an error in this case now, but I'd like to avoid these kinds of mistakes if possible.)

I think it would be simpler to just use uint32 instead (though looking it up, it seems the kernel actually defines umode_t as unsigned short which is uint16).

@cyphar
Copy link
Owner

cyphar commented Dec 22, 2024

Given that libpathrs takes os.FileMode (and translates the mode to the unix mode internally), it might be a good idea to move this to be os.FileMode (so that users can more easily migrate to libpathrs later) but this will require adding some translation code.

This will require a 0.4 release though, since this is a breaking change.

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