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

Parameters for chown should be signed integers not unsigned #15828

Merged
merged 7 commits into from Jul 26, 2019

Conversation

nicholi
Copy link
Contributor

@nicholi nicholi commented Jul 25, 2019

Fixes #10748

The calls for chown, lchown, fchown, and fchownat (from Mono.Unix.Native.Syscall) are all declared with unsigned integers for the owner and group parameters. This is incorrect.

Directly from the manpage
man 2 chown


...
       int chown(const char *pathname, uid_t owner, gid_t group);
       int fchown(int fd, uid_t owner, gid_t group);
       int lchown(const char *pathname, uid_t owner, gid_t group);

...

       int fchownat(int dirfd, const char *pathname,
                    uid_t owner, gid_t group, int flags);

...
...
       If the owner or group is specified as -1, then that ID is not changed.
...

Emphasis added on the documentation explaining how the parameters can take the value -1, clearly not an unsigned integer. The uid_t struct is not an unsigned integer. I am assuming this was just an oversite on whoever re-wrote Mono.Unix.Native.Syscall.

The original calls from Mono.Posix.Syscall (now marked as Obsolete) all correctly use signed integers.

                [DllImport ("libc", SetLastError=true)]
                public static extern int chown (string path, int owner, int group);
                [DllImport ("libc", SetLastError=true)]
                public static extern int lchown (string path, int owner, int group);

Even the documentation directly from mcs/class/Mono.Posix/Documentation/en/Mono.Unix.Native/Syscall.xml

          <para>
            One of the owner or group id's
            may be left unchanged by specifying it as -1.
          </para>

I have changed the signatures from uint to int, the relevant XML documentation, and also 3 usages of chown within the project itself (switch from Convert.ToUInt32 to Convert.ToInt32). Hopefully this PR won't sit here for years, but who knows.

@dnfclas
Copy link

dnfclas commented Jul 25, 2019

CLA assistant check
All CLA requirements met.

@akoeplinger
Copy link
Member

@monojenkins build

@nicholi
Copy link
Contributor Author

nicholi commented Jul 25, 2019

Can't believe I totally spaced on the return types there.

@akoeplinger
Copy link
Member

@nicholi it does introduce an API breaking change though so I think we'll need to add an overload for the existing uint-taking public methods.

@nicholi
Copy link
Contributor Author

nicholi commented Jul 25, 2019

@akoeplinger Semi-related, would it be posible to request a more recent build uploaded to NuGet?

https://www.nuget.org/packages/Mono.Posix.NETStandard

The last one was quite awhile ago. Though it doesn't look like too many serious changes have been committed since then. Other than this one possibly.

@nicholi
Copy link
Contributor Author

nicholi commented Jul 25, 2019

@nicholi it does introduce an API breaking change though so I think we'll need to add an overload for the existing uint-taking public methods.

Yeah I wasn't too sure on that, since the uint versions were technically incorrect all along. Re-added them back in marked regions, and also with Obsolete attribute. That seems most appropriate correct?

@akoeplinger
Copy link
Member

Re-added them back in marked regions, and also with Obsolete attribute. That seems most appropriate correct?

Yes that looks fine.

would it be posible to request a more recent build uploaded to NuGet?

I'll ping the relevant people after this was merged.

@akoeplinger
Copy link
Member

@monojenkins approve

@akoeplinger
Copy link
Member

@monojenkins commit apidiff

monojenkins added a commit to mono/api-snapshot that referenced this pull request Jul 26, 2019
@akoeplinger akoeplinger merged commit 51aa4b0 into mono:master Jul 26, 2019
@akoeplinger
Copy link
Member

@nicholi thank you for the contribution!

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.

Mono.Posix: Syscall.chown has incorrect signature
5 participants