Austin Group Defect Tracker

Aardvark Mark IV

Main | My View | View Issues | Change Log | Docs
  

Viewing Issue Simple Details [ Jump to Notes ] [ Issue History ] [ Print ]
ID Category Severity Type Date Submitted Last Update
0001283 [1003.1(2016/18)/Issue7+TC2] System Interfaces Objection Clarification Requested 2019-09-04 09:16 2019-12-05 11:20
Reporter geoffclare View Status public  
Assigned To
Priority normal Resolution Accepted As Marked  
Status Applied  
Name Geoff Clare
Organization The Open Group
User Reference
Section chmod()
Page Number 665
Line Number 22780
Interp Status Approved
Final Accepted Text See Note: 0004592.
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 tc3-2008
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.
(0004592)
Don Cragun (manager)
2019-10-03 16:04
edited on: 2019-10-03 16:21

 The standard is unclear on this issue, and no conformance distinction can be made between alternative implementations based on this. This is being referred to the sponsor.

Rationale:
-------------
The selected option matches current existing practice, but alternative implementations have been investigated.

Notes to the Editor (not part of this interpretation):
-------------------------------------------------------
Implement OPTION 1 from the Desired Action, but change:
the st_mode value returned by stat() but cannot
to:
the st_mode value returned by lstat() or stat() but cannot
(0004593)
shware_systems (reporter)
2019-10-03 16:28

 Re: 4551
Per the phone discussion 2019-10-03, there is no S_IFMT value reserved as unavailable, so a total of 16 types could be encoded in it and still fit in a short. Per the Future Directions section any use by an implementation of other values, including a reservation of 0 for any purpose, in S_IFMT are non-portable. To show a stat structure does not represent any actual file, an implementation would need to define an S_TYPEISxxx macro for testing that state; an S_IFMT value of 0 does not serve this purpose.
(0004594)
eblake (manager)
2019-10-03 16:30

 Option 3 is less like existing practice. The following example on Linux is proof that setting S_IFLNK bits in st.st_mode before calling chmod() on S_IFREG is ignored, not rejected:

$ cat file.c
#include <stdio.h>
#include <errno.h>
#include <sys/stat.h>

int main(void)
{
  struct stat st;
  lstat("b", &st);
  int i = chmod("a", st.st_mode);
  printf ("%d %d\n", i, errno);
}
$ touch a
$ ln -s a b
$ ./a.out
0 0
(0004595)
kre (reporter)
2019-10-04 21:57

 Re Note: 0004593

I have no idea why the phone call even wasted time discussing that, as it
is completely off topic for the issue at hand, but ... I agree, POSIX does not
reserve a value for *nothing* as a file type - why would it? By definition
such a thing does not exist, and so cannot be returned by any of the stat()
functions, and would be just one of many (presumably) invalid values to hand to
mknod() if posix even defined that interface, so calling that one out there
would make no sense either.

That said, I have never seen an implementation where (st_mode & S_IFMT) == 0
represented anything, and I cannot imagine a situation where an implementation
would ever do that ... at the very least, that would be the last S_IFMT value
assigned, and whenever values are being assigned from a potentially unbounded
set (ie: where we cannot prove we will never need any more) and you're reaching
the end of the previously allocated space, some method needs to be found to
add new values in another way. Since the implementation knows it is going to
need to do that, and probably soon(ish) it is far more reasonable to simply do
it now (whenever that "now" is) rather that assigning 0 and deferring the work
until later.

POSIX has recognised that, and made it clear that whatever other way that
implementations choose will be OK (the tests for file types for new file
types will never be expected to work with only st_mode as the arg, but only
with the whole structure available), so why would an implementation use
that one, never before used value, even though it might poausibly have
been use as the stating point, for regular files - but wasn't - for anything?

But as I said, all of this is off topic for chmod, which explicitly is not
touching any of those bits, and completely irrelevant to anything here.

Re Note: 0004594 .. I agree, that is the common implementation, and I certainly
would not have disallowed that. I thought it was clear, that my only point
was to allow an implementation to give an error in that case if it wants to.
That is, I see no particular benefit in allowing the st_mode from one file
being applied to another, without masking out the non-permission bits. I doubt
this is a common programming idiom.

All that said, I have no problem with the choice of option 1 if that is what
everyone believes is the best way.
(0004597)
agadmin (administrator)
2019-10-07 15:15

 Interpretation proposed: 7 October 2019
(0004643)
agadmin (administrator)
2019-11-11 12:19

 Interpretation Approved: 11 Nov 2019

- 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
2019-10-03 16:04 Don Cragun Note Added: 0004592
2019-10-03 16:04 Don Cragun Status New => Interpretation Required
2019-10-03 16:04 Don Cragun Resolution Open => Accepted As Marked
2019-10-03 16:04 Don Cragun Final Accepted Text => See Note: 0004592.
2019-10-03 16:05 Don Cragun Tag Attached: tc3-2008
2019-10-03 16:05 Don Cragun Interp Status --- => Pending
2019-10-03 16:20 Don Cragun Note Edited: 0004592
2019-10-03 16:21 Don Cragun Note Edited: 0004592
2019-10-03 16:28 shware_systems Note Added: 0004593
2019-10-03 16:30 eblake Note Added: 0004594
2019-10-04 21:57 kre Note Added: 0004595
2019-10-07 15:15 agadmin Interp Status Pending => Proposed
2019-10-07 15:15 agadmin Note Added: 0004597
2019-11-11 12:19 agadmin Interp Status Proposed => Approved
2019-11-11 12:19 agadmin Note Added: 0004643
2019-12-05 11:20 geoffclare Status Interpretation Required => Applied

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