Austin Group Defect Tracker

Aardvark Mark III


Viewing Issue Simple Details Jump to Notes ] Issue History ] Print ]
ID Category Severity Type Date Submitted Last Update
0001283 [1003.1(2016)/Issue7+TC2] System Interfaces Objection Clarification Requested 2019-09-04 09:16 2019-09-05 08:47
Reporter geoffclare View Status public  
Assigned To
Priority normal Resolution Open  
Status New  
Name Geoff Clare
Organization The Open Group
User Reference
Section chmod()
Page Number 665
Line Number 22780
Interp Status ---
Final Accepted Text
Summary 0001283: should chmod() ignore file type bits in st_mode
Description The description of chmod() says:
The chmod() function shall change S_ISUID, S_ISGID, [XSI]S_ISVTX,[/XSI] and the file permission bits of the file named by the pathname pointed to by the path argument to the corresponding bits in the mode argument.

The way this is worded implies that only the specified bits in the mode argument are examined by chmod(), and it should ignore other bits. However, there is also a "may fail" EINVAL error:
The value of the mode argument is invalid.

whose presence could be seen as casting doubt on this interpretation. Alternatively, perhaps this EINVAL error is there so that non-XSI implementations that don't support S_ISVTX can fail the chmod() if an application attempts to set that bit.

Another consideration is whether any implementations support additional settable bits as an extension. If such extensions are allowed, the question becomes whether chmod() should ignore the bits used to encode the file type (and any other "read-only" bits that can be set in st_mode).

This affects code which modifies file permissions by obtaining the st_mode value for a file and setting or clearing some permission bits in the value before passing it to chmod(). For example:
stat(file, &sbuf); chmod(file, sbuf.st_mode | S_IRWXU);
If chmod() is supposed to ignore file-type (and other read-only) st_mode bits then this is safe, since any other bits outside 07777 that are set in st_mode would correspond to additional settable bits supported as an extension.

I have some code which does something like this and it has worked fine for many years on many systems. However, recently a system failed the chmod() with EINVAL.

Is this code supposed to be portable, or is it relying on implementations choosing not to fail with EINVAL if read-only st_mode bits are set the requested mode?
Desired Action OPTION 1

On page 665 line 22779 section chmod(), change:
The chmod() function shall change S_ISUID, S_ISGID, [XSI]S_ISVTX,[/XSI] and the file permission bits of the file named by the pathname pointed to by the path argument to the corresponding bits in the mode argument. The application shall ensure that the effective user ID of the process matches the owner of the file or the process has appropriate privileges in order to do this.
to:
The chmod() function shall change S_ISUID, S_ISGID, [XSI]S_ISVTX,[/XSI] and the file permission bits of the file named by the pathname pointed to by the path argument to the corresponding bits in the mode argument. If any bits that can be set in the st_mode value returned by stat() but cannot be changed using chmod(), such as the bits that are used to encode the file type, are set in the mode argument, these read-only st_mode bits shall be ignored.

If the effective user ID of the process does not match the owner of the file and the process does not have appropriate privileges, the chmod() function shall fail.

On page 666 line 22834 section chmod(), change:
The value of the mode argument is invalid.
to:
The value of the mode argument, ignoring read-only st_mode bits (see the DESCRIPTION), is invalid.

On page 667 line 22866 section chmod() add a new example:
Modifying File Permissions

The following example adds group write permission to the existing permission bits for a file if that bit is not already set.
#include <sys/stat.h>

struct stat sbuf;
...
if (stat(path, &sbuf) == 0 && (sbuf.st_mode & S_IWGRP) == 0)
    chmod(path, sbuf.st_mode | S_IWGRP);


OPTION 2

On page 665 line 22779 section chmod(), change:
The chmod() function shall change S_ISUID, S_ISGID, [XSI]S_ISVTX,[/XSI] and the file permission bits of the file named by the pathname pointed to by the path argument to the corresponding bits in the mode argument. The application shall ensure that the effective user ID of the process matches the owner of the file or the process has appropriate privileges in order to do this.
to:
The chmod() function shall change S_ISUID, S_ISGID, [XSI]S_ISVTX,[/XSI] and the file permission bits of the file named by the pathname pointed to by the path argument to the corresponding bits in the mode argument. If any other bits are set in the mode argument, the chmod() function may treat the mode argument value as invalid; if the implementation does not support the XSI option, setting the 01000 bit may also cause the mode value to be treated as invalid (01000 is the value of S_ISVTX on implementations that support the XSI option).

If the effective user ID of the process does not match the owner of the file and the process does not have appropriate privileges, the chmod() function shall fail.

On page 667 line 22866 section chmod() add a new example:
Modifying File Permissions

The following example adds group write permission to the existing permission bits for a file if that bit is not already set.
#include <sys/stat.h>

struct stat sbuf;
...
if (stat(path, &sbuf) == 0 && (sbuf.st_mode & S_IWGRP) == 0)
    chmod(path, (sbuf.st_mode & 07777) | S_IWGRP);

On page 667 line 22878 section chmod() add a new first paragraph to APPLICATION USAGE:
When adding or removing permission bits in a file's mode, the st_mode value obtained from, for example, stat() should be masked with 07777 in order to ensure an [EINVAL] error does not occur.
Tags No tags attached.
Attached Files

- Relationships

-  Notes
(0004547)
shware_systems (reporter)
2019-09-04 17:10

This was discussed years ago, due to confusion I had about encoding device types in st_mode. Having additional bits in st_mode is not a conforming extension, to keep it compatible with type short, though it's arguable the ISVTX bit may have a different label and function on non-XSI systems. The means of extending stat left open is to define a new field, not add bits. Then set/test macros that take the entire struct as argument and not the mode field alone, are to be defined, similar to the test for being TYM or SHM, to hide non-portable naming.

I believe the reason for EINVAL being may fail is some device types may not support the particular function a bit designates; for example, a read only FIFO may consider it invalid to attempt to set any W or X permission bits.
(0004548)
kre (reporter)
2019-09-04 22:40

Re bugnote: 4547

There is nothing in posix (nor in some implementations) that suggests that
a mode_t should be 16 bits (short). Adding bits is entirely possible (and
needed to define new file types, if not so much for more permissions, which
would be hard to do in a portable manner).

Re the proposed resolution: I don't much like using 07777 as a "magic"
number, even though the values of all the relevant bits are defined, and
fit (exactly) into that value. Nor am I convinced that giving in to
whatever implementation returned EINVAL is necessarily the corrcet thing
to do.

What would seem more reasonable to me would be to require implementations
accept either all 0's in the "other" bits, or a value that exactly matches
the current settings of those bits, and is permitted to return EINVAL only
in cases where the application has attempted to use chmod to alter the state
of those bits (if it isn't already required somewhere, I'd also add, somewhere,
not related to chmod, a requirement that it is invalid to have all the other
bits 0 for any existing file, whatever its type). EINVAL in cases where an
attempt is made to set permissions that are impossible for the file in question
is OK as well of course.
(0004549)
kre (reporter)
2019-09-04 23:03

OPTION 3:

On page 665 line 22779 section chmod(), change:
    [the same words as in options 1 & 2]
to
    The chmod() function shall change S_ISUID, S_ISGID, [XSI]S_ISVTX,[/XSI]
    and the file permission bits of the file named by the pathname pointed to
    by the path argument to the corresponding bits in the mode argument.
    Any other bits set in the mode argument that differ from the value that
    would be returned in st_mode value returned by stat() of the same path,
    may be treated as an error.

    If the effective user ID of the process does not match the owner of the
    file and the process does not have appropriate privileges, the chmod()
    function shall fail.


On page 666 line 22834 section chmod(), change nothing.

Add the same new example on page 667 line 22866 as is shown for OPTION 1.
(0004550)
shware_systems (reporter)
2019-09-05 01:48

Re: 4548
It is not that it has to be a short, just that what is designated for the type can fit in it and no more file types or mode bits will be added to break that possibility. I was thinking similarly to you, actually, that the intent was it might be extended, with each new file type being represented by its own bit (so testing for or excluding multiple types in one mask/compare was plausible) but this is not the case.
(0004551)
kre (reporter)
2019-09-05 03:46

st_mode in original unix (pdp-11) was an int (16 bits). When ported to
32 bit systems a choice needed to be made .. keep it an int (now 32 bits)
preserving source code compat, or make it short (u_short) keeping binary
compat with the older version. Either choice would have been reasonable,
and it is possible both were made by different implementations. BSD systems
have mode_t as uint32_t (I think all of them, but did not check.)

Representing file types as individual bits was never going to happen, that
ship sailed in the earliest implementations, but adding more new file types
is certainly still possible (many more have been added to the original set
over the years) and any implementation is free to add as many as it likes.
With only 4 bits left available in a short, there's only room for 15 different
file types (reserving 0 as unavailable), but with wider mode_t many more
can be handled - not that I know of any systems with more than 15.

An implementation, using interfaces that are POSIX extensions could also
place other info in the st_mode field - as long as the access methods defined
by the standard still all work, I see no issue with that.

None of that is relevant to chmod() though, which only ever affects those
bottom 12 bits. I think we should keep the standard such that giving a mode
arg to chmod that is < (1<<12) and which contains nothing inappropriate for
the actual file type, should always be valid (I don't believe there's any
question about that one), but I also believe it should be OK to use the value
obtained from st_mode after a stat() while changing any of those bottom 12 bits.
That would be option 1, except I also think it reasonable to allow an
implementation to reject the mode (EINVAL) if any of the other bits from
the st_mode value were altered (so, the "ignored" in option 1 is not my
choice). Note that is "allowed" to the implementation, not required of it.
If an implementation decides to simply ignore all but the bottom 12 bits that
is OK too.
(0004552)
kre (reporter)
2019-09-05 04:04

Just in case it is not clear, I know that the standard is not going
to require more that 16 bit mode_t and that new file types defined
by POSIX will be defined in a way that makes the implementation more
flexible ... but for chmod() purposes, that is irrelevant - what matters
is that implementations with a wider mode_t can choose to add new file
types there - both standard and non-standard ones. Code that manipulates
the st_mode field returned from stat() needs to be aware of that. That
includes code planning on doing chmod(). There are 2 choices for such
code, either pass only the bottom 12 bits of the mode_t value as the mode
arg to chmod(), or pass the entire st_mode changing only some of the bottom
12 bits. Either of those 2 should be specified to work. Any other
manipulation must be permitted to fail (but not required to fail).
(0004553)
geoffclare (manager)
2019-09-05 08:47
edited on: 2019-09-05 09:01

Re: Note: 0004549 I can see a few of problems with option 3:

* It can't just talk about the st_mode that would be returned by stat(); it needs to relate it to stat() for chmod() and for fchmodat() with flag 0, and to lstat() for fchmodat() with flag AT_SYMLINK_NOFOLLOW.
(I realise now that option 1 also has a similar problem, but it can be fixed there just by changing stat() to lstat() in "can be set ... by stat()".)

* If an application clears one or more bits in the part of st_mode that encodes the file type while leaving at least one of those bits set, I think your intention is that this is allowed to be an error, but it's not clear from the wording. Maybe this would be clearer:
If the value represented by all of the other bits in the mode argument is non-zero and differs from the value that those bits would represent in the st_mode value returned by ... of the same path, this may be treated as an error.

* There's no point changing the DESCRIPTION to narrow down the set of mode values that can be treated as an error if the EINVAL wording is left as-is, because it currently gives implementations carte blanche.


- Issue History
Date Modified Username Field Change
2019-09-04 09:16 geoffclare New Issue
2019-09-04 09:16 geoffclare Name => Geoff Clare
2019-09-04 09:16 geoffclare Organization => The Open Group
2019-09-04 09:16 geoffclare Section => chmod()
2019-09-04 09:16 geoffclare Page Number => 665
2019-09-04 09:16 geoffclare Line Number => 22780
2019-09-04 09:16 geoffclare Interp Status => ---
2019-09-04 17:10 shware_systems Note Added: 0004547
2019-09-04 22:40 kre Note Added: 0004548
2019-09-04 23:03 kre Note Added: 0004549
2019-09-05 01:48 shware_systems Note Added: 0004550
2019-09-05 03:46 kre Note Added: 0004551
2019-09-05 04:04 kre Note Added: 0004552
2019-09-05 08:47 geoffclare Note Added: 0004553
2019-09-05 09:00 geoffclare Note Edited: 0004553
2019-09-05 09:01 geoffclare Note Edited: 0004553


Mantis 1.1.6[^]
Copyright © 2000 - 2008 Mantis Group
Powered by Mantis Bugtracker