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

convert negative uid and gid to 0o7777777 #4102

Closed

Conversation

philaris
Copy link
Contributor

@philaris philaris commented Dec 26, 2022

Substitute negative uid and gid that is returned from os.Getuid
and os.Getgid in Windows with maximum value that is allowed in
v7 and ustar tar formats, i.e., 0o7777777. See:
https://www.gnu.org/software/tar/manual/html_section/Formats.html

This fixes the issue of a 32-bit binary restic dump that creates
a tar file containing negative uid and gid files:
#4101

What does this PR change? What problem does it solve?

#4101

Was the change previously discussed in an issue or on the forum?

I have only created the relevant issue #4101.

Checklist

Comments on checklist: I did not write a new test, because the change is simple. I have run the tests though. The change is internal, so there is no change in the manual.

  • I have read the contribution guidelines.
  • I have enabled maintainer edits.
  • I have added tests for all code changes.
  • I have added documentation for relevant changes (in the manual).
  • There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
  • I have run gofmt on the code in all commits.
  • All commit messages are formatted in the same style as the other commits in the repo.
  • I'm done! This pull request is ready for review.

Substitute negative uid and gid that is returned from os.Getuid
and os.Getgid in Windows with maximum value that is allowed in
v7 and ustar tar formats, i.e., 0o7777777. See:
https://www.gnu.org/software/tar/manual/html_section/Formats.html

This fixes the issue of a 32-bit binary restic dump that creates
a tar file containing negative uid and gid files:
restic#4101
@greatroar
Copy link
Contributor

Good catch, but I think fixing the dump command is a better idea. This changes the repo format, so it won't work with existing repositories.

@philaris
Copy link
Contributor Author

Thanks @greatroar. Unless I am missing something, I have not changed anything in the repository format. The uid and gid in the node are still of type uint32. In general, the repository format has no provision for negative uid and gid and this has not changed.

As for your suggestion for changing dump, I agree that it is a good idea and can be done orthogonally to this pull request. One has to decide what to do in case the uid/gid is more than what can be stored in an int32 type. The main issue is the discrepancy between uint32 in restic and the int type of Uid/Gid in https://pkg.go.dev/archive/tar#Header. Maybe I should create another issue for that.

@greatroar
Copy link
Contributor

They're still uint32, but the meaning of that uint32 for dumps from Windows changes. It's not documented anywhere that ^uint32(0) means -1, but there are many repos where that is the case.

@philaris
Copy link
Contributor Author

philaris commented Dec 27, 2022

I see that with the code in this pull request the repository node that is created is different than before. If some person's code relies on the specific old value of 4294967295, then there could be a problem. Should this person's code rely on this specific value? Probably not.

Moreover, if you backup an actual file of the Windows filesystem and not stdin, the uid and gid are set to 0. Continuing the example in #4101:

> echo test1 > test1.txt
> restic_0.14.0_windows_386 --password-file=testpassfile.txt --repo=repo backup test1.txt

Here is the file snapshot:

> restic_0.14.0_windows_386 --password-file=testpassfile.txt --repo=repo ls -l 096e1ad0
repository 1a634ddc opened (repository version 2) successfully, password is correct
snapshot 096e1ad0 of [D:\code\testrestic\test1.txt] filtered by [] at 2022-12-26 13:52:39.5125871 +0100 CET):
-rw-rw-rw-     0     0      8 2022-12-26 13:51:56 /test1.txt

Here is the stdin snapshot:

> restic_0.14.0_windows_386 --password-file=testpassfile.txt --repo=repo ls -l c7afaf40
repository 1a634ddc opened (repository version 2) successfully, password is correct
snapshot c7afaf40 of [D:\stdin] filtered by [] at 2022-12-26 09:30:25.2227589 +0100 CET):
-rw-r--r-- 4294967295 4294967295     13 2022-12-26 09:30:25 /stdin

Is it nice to have such a uid/gid discrepancy between a normal file backup and a stdin backup in Windows? Maybe both should have uid/gid of 0. Maybe zero is also a better value for this commit (instead of 0o7777777).

@MichaelEischer
Copy link
Member

From a quick look around Linux, FreeBSD, Solaris (https://docs.oracle.com/cd/E19082-01/820-0543/gfraf/index.html) and macOS the standard seems to be uid_t == uint32 nowadays. Thus, we can assume that (uid_t) -1 == uint32(-1). According to https://systemd.io/UIDS-GIDS/ a uid of -1 is not a valid uid anyways, e.g. chown turns into a no-op for uid -1.

Even the POSIX man page for getuid() states that It is possible for implementations to provide an extension where a process in a non-conforming environment will not be associated with a user or group ID. It is recommended that such implementations return (uid_t)-1 and set errno to indicate such an environment. Thus, it should be reasonably safe to just convert -1 to uid 0.

As uids above 2^31 are apparently a not particularly portable mess, I'd prefer to keep the fix minimally invasive and just convert from uid -1 to 0 when dumping to a tar file. That way the repository also won't loose the information that the uid is sort of undefined.

Is it nice to have such a uid/gid discrepancy between a normal file backup and a stdin backup in Windows? Maybe both should have uid/gid of 0. Maybe zero is also a better value for this commit (instead of 0o7777777).

stdin-backups fall back to the uid / gid of the current user. On Windows this just defaults to -1. After fixing the dump command, the current behavior should work fine, so it's probably not worth the effort of changing it.

@philaris
Copy link
Contributor Author

Thanks for the answer @MichaelEischer! So we can close this and maybe move to #4103.

@MichaelEischer
Copy link
Member

Yes, let's take the route of #4103 instead (although still limited to only the -1 uid)

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