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
0000220 [1003.1(2008)/Issue 7] Base Definitions and Headers Editorial Enhancement Request 2010-02-19 23:14 2024-06-11 08:53
Reporter EdSchouten View Status public  
Assigned To ajosey
Priority normal Resolution Accepted As Marked  
Status Closed  
Name Ed Schouten
Organization
User Reference
Section sys/select.h
Page Number n/a
Line Number n/a
Interp Status ---
Final Accepted Text Note: 0005994
Summary 0000220: FD_ISSET can use const keyword
Description A quote from the description of <sys/select.h>:

The following shall be declared as functions, defined as macros, or both. If functions are declared, function prototypes shall be provided.
[...]
int FD_ISSET(int, fd_set *);

The FD_ISSET() macro or function doesn't need to modify the fd_set, which means the second argument can use a const keyword as well.
Desired Action Change the definition to:

int FD_ISSET(int, const fd_set *);
Tags issue8
Attached Files

- Relationships
has duplicate 0001032Closed 1003.1(2013)/Issue7+TC1 FD_ISSET() should take a const fd_set * 

-  Notes
(0000390)
geoffclare (manager)
2010-02-22 15:58

 If this change is accepted, then an equivalent change should also be made to the pselect() page in XSH.
(0000394)
msbrown (manager)
2010-03-04 16:44

 This is one of the few macros that are specified as "macro only"; the "const" change would only be useful if there were an underlying function definition to take advantage of it.

If desired you can submit a proposal to replace this macro with a function as a new issue entry.
(0000397)
msbrown (manager)
2010-03-04 22:47

 Correcting the previous note for the record:
What I should have written is that FD_ISSET() _can_ _be_ only a macro. I.e. there doesn't have to be an underlying function.

The reason the requested change is not worth doing mainly derives
from this statement on the pselect() page:
    [...] FD_CLR(), FD_ISSET(), FD_SET(), and FD_ZERO(). It is
    unspecified whether each of these is a macro or a function. If a
    macro definition is suppressed in order to access an actual
    function [...] the behavior is undefined.
(0003110)
nick (manager)
2016-03-24 16:29
edited on: 2016-03-24 16:30

 Reopening in the light of 0001032
(0003114)
shware_systems (reporter)
2016-03-31 18:59
edited on: 2016-03-31 19:01

 For these, when defined as macros they look like:
#define FD_CLR(fd, fdsetp) macrodef
#define FD_ISSET(fd, fdsetp) macrodef
#define FD_SET(fd, fdsetp) macrodef
#define FD_ZERO(fdsetp) macrodef
where macrodef is an (int)(expression), for FD_ISSET, where (expression) may or may not reference an actual interface with prototype
int name(int, fd_set*) or int name(int, const fd_set*).
The standard is what it is in that an actual function declaration shall use the first prototype if it does not want to #define a wrapper. Use of the second prototype requires being wrapped by a macro definition, as things are.

Those implementations using only the second option need to include documentation that use of FD_ISSET shall use a reference declared const for fdsetp, not make it part of the definition, it looks to me.
Example:
in <select.h>;
typedef long long fd_set;
#define FD_ISSET(fd, fdsetp) (int)(do_fd_isset(fd, fdsetp))
extern int do_fd_isset(int fd, const fd_set *fdsetp)
    { return (int)(*fdsetp & (1<<fd)); }

in examp.c file;
#include <sys/select.h>
int fd=0; // stdin
fd_set fdset; // actual structure, nominally a long long here
FD_ZERO(&fdset); // just init it
const fd_set* fdsetp=&fdset; // declare const alias
int set_fd=FD_ISSET(fd, fdsetp); // returns 0

or
fd_set* fdsetp=&fdset;
int set_fd=FD_ISSET(fd, (const) fdsetp); // returns 0

should work also, as it expands to
(int)(do_fd_isset(fd, (const) fdsetp))

Then an application note could say some implementations require that type of casting, but such a requirement is an extension. IMO, those implementations should just provide another macro, nominally FD_ISSETC(fd, fdsetp), for wrapping the second prototype.
(0003115)
EdSchouten (updater)
2016-04-01 07:48

 Sorry, but I don't entirely understand where you're going with this. I think the question we should be focussing on is as follows:

Do we expect that this code compiles on any POSIX-compliant system?

#include <sys/select.h>
int my_isset(int fd, const fd_set *fds) {
  return FD_ISSET(fd, fds);
}

- If the answer is yes, then the text in the standard should be adjusted to use "const fd_set *" instead of "fd_set *".
- If the answer is no, then it should remain the way it is.

It's that simple. Whether FD_ISSET() is implemented as a function or a macro (or both, or a macro invoking a function, or ...) is completely irrelevant in this discussion. The concept of constness would apply to any implementation.
(0003117)
shware_systems (reporter)
2016-04-01 14:30

 The answer is 'no', in my opinion, to keep backwards compatibility.

What I was getting at is showing how the standard leaves 'yes' open as an extension, and providing a possible addition to the pselect() Rationale of various ways how such an extension might be implemented. The clause about undefined behavior if the macro suppressed becomes implementation specific behavior. The FD_ISSETC suggestion would be a possible backwards compatible addition to Issue 8, not a replacement, if some implementation adds it to their <sys/select.h> header.
Another possibility is requiring FD_ISSET to be a macro of the form:
#define FD_ISSET(fd, fdsetp, ...) macrodef
where the ... would be, when a parameter is specified, a flag to treat fdsetp as const in macrodef. When used like currently specified fdsetp would be non-const.
(0003118)
EdSchouten (updater)
2016-04-01 15:01

 Instead of handwaving about keeping backwards compatibility, I think it makes sense to take a look at actual implementations. Results so far:

Systems that I tested that allow fd_set to be const: FreeBSD, Linux, Mac OS X, NetBSD
Systems whose header files I inspected that allow fd_set to be const: OpenBSD, DragonFly BSD, Solaris
Systems that I found that don't allow fd_set to be const: none
(0003121)
shware_systems (reporter)
2016-04-04 14:45

 Bug 1032 mentions OpenBSD breaking a couple apps; so 'none' is questionable.

The generic glibc version, which is what some of those tested systems may be using, effectively implements:
inline _Bool FD_ISSET(atomic unsigned long long int fd, fd_set *fdset)

so is not strictly conforming. Implicit casting hides int's aren't explicitly used, and the inline usage of fdset is only as an rvalue so the compiler rightfully doesn't care if it's const qualified or not. It doesn't qualify as non-conforming, though; it's just optimized for speed over security and strictness. The inline code also assumes the application using it has extra code for range checking fd, or is compiled with array index range check extensions enabled, before any call. If this isn't done it may core dump a system or raise SIGSEGV, which I doubt is sufficiently documented. The atomic qualifier is due to fd is evaluated multiple times so should not be an expression with side effects.

I agree this functionality is desirable as helping to plug security holes like this, however unlikely they're a factor in existing practice, but the cost of backwards compatibility for Issue 8 is minimal; a few apps adding a single 'C' to a wrapper like in Note 3115. This is more likely to be approved than requiring a lot more apps, potentially, to factor in const modifiers to their code because they followed the standard as it is now, where the same variable declaration is usable with all the FD_* macros/ interfaces.
(0003122)
EdSchouten (updater)
2016-04-04 14:52

 If you read 0001032 closely and check the OpenBSD CVS logs, you'll see that OpenBSD switched to a version that used a static inline function where they did *not* use 'const fd_set *'. This cause things to break. This is why they had to switch over to using 'const fd_set *' and filed the bug in this bug tracker.
(0003123)
shware_systems (reporter)
2016-04-04 16:06

 That is not a bug in the standard; those applications did not take into account some implementations may elect to use a method that does stricter parameter validation than the platform the app was originally developed on. The standard elected to not force the overhead of that validation on all implementations, by not requiring function prototypes as it does with most other interfaces. It can be said the standard overlooked mentioning an additional form of FD_ISSET was desirable as an extension for implementers to provide to give applications the option, and OpenBSD can still do this, but that's all.
(0005994)
geoffclare (manager)
2022-10-17 15:59

 Make the change in the desired action, and also the following change:

On 2018 edition page 1553 line 50827 section pselect(), change:
int FD_ISSET(int fd, fd_set *fdset);
to:
int FD_ISSET(int fd, const fd_set *fdset);

- Issue History
Date Modified Username Field Change
2010-02-19 23:14 EdSchouten New Issue
2010-02-19 23:14 EdSchouten Status New => Under Review
2010-02-19 23:14 EdSchouten Assigned To => ajosey
2010-02-19 23:14 EdSchouten Name => Ed Schouten
2010-02-19 23:14 EdSchouten Section => sys/select.h
2010-02-19 23:14 EdSchouten Page Number => n/a
2010-02-19 23:14 EdSchouten Line Number => n/a
2010-02-22 15:58 geoffclare Note Added: 0000390
2010-03-04 16:44 msbrown Interp Status => ---
2010-03-04 16:44 msbrown Note Added: 0000394
2010-03-04 16:44 msbrown Status Under Review => Closed
2010-03-04 16:44 msbrown Resolution Open => Rejected
2010-03-04 16:44 msbrown Category Rationale => Base Definitions and Headers
2010-03-04 22:47 msbrown Note Added: 0000397
2016-03-24 16:27 nick Relationship added has duplicate 0001032
2016-03-24 16:29 nick Note Added: 0003110
2016-03-24 16:29 nick Status Closed => Under Review
2016-03-24 16:29 nick Resolution Rejected => Open
2016-03-24 16:30 nick Note Edited: 0003110
2016-03-24 16:31 nick Resolution Open => Reopened
2016-03-31 18:59 shware_systems Note Added: 0003114
2016-03-31 19:01 shware_systems Note Edited: 0003114
2016-04-01 07:48 EdSchouten Note Added: 0003115
2016-04-01 14:30 shware_systems Note Added: 0003117
2016-04-01 15:01 EdSchouten Note Added: 0003118
2016-04-04 14:45 shware_systems Note Added: 0003121
2016-04-04 14:52 EdSchouten Note Added: 0003122
2016-04-04 16:06 shware_systems Note Added: 0003123
2022-10-17 15:59 geoffclare Note Added: 0005994
2022-10-17 16:00 geoffclare Final Accepted Text => Note: 0005994
2022-10-17 16:00 geoffclare Status Under Review => Resolved
2022-10-17 16:00 geoffclare Resolution Reopened => Accepted As Marked
2022-10-17 16:00 geoffclare Tag Attached: issue8
2022-11-01 14:55 geoffclare Status Resolved => Applied
2024-06-11 08:53 agadmin Status Applied => Closed

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