Re: [PATCH v2 0/6] fix wrong program abort on __FD_ELT

["Carlos O'Donell" <carlos at redhat dot com>]

On 03/29/2013 11:17 AM, kosaki.motohiro@gmail.com wrote:
> From: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
> 
> Currently, FD_SET, FD_CLR and FD_ISSET make program abort when
> passing >__FD_SETSIZE value and _FORTIFY_SOURCE is greater than 0.
> 
> However it is wrong. Linux accept BSD style dynamic fd table allocations
> likes below over 10 years. 
> 
>     http://netbsd.gw.com/cgi-bin/man-cgi?select++NetBSD-4.0
> 
> 		   fd_set *fdsr;
>                    int max = fd;
> 
>                    fdsr = (fd_set *)calloc(howmany(max+1, NFDBITS),
>                        sizeof(fd_mask));
>                    if (fdsr == NULL) {
>                            ...
>                            return (-1);
>                    }
>                    FD_SET(fd, fdsr);
>                    n = select(max+1, fdsr, NULL, NULL, &tv);
>                    ...
>                    free(fdsr);
> 
> Moreover this technique is already in use multiple applications. And unfortunately, Ubuntu turn on _FORTIFY_SOURCE=2 by default and then user can hit this issue easily if bump up RLIMIT_NOFILE.
> 
> This patch series restrict fd argument check to only work when _STRICT_FD_SIZE_CHECK is set.

(1) Enable check by default, but allow applications to turn it off
    at build time.

I have been looking over this code and talking to Florian Weimer about this.

I don't like _FORTIFY_SOURCE checks for FD_SETSIZE because they ignore 
existing practice, and that when these changes went into glibc we provided 
no alternative for applications that were using established patterns from 
BSD and related OSs.

However, glibc implements POSIX, and follows the definitions of these
macros and functions as outlined in POSIX. Following a standard is the
only sensible way we will get an implementation that operates as expected
(by default).

So while I think that this patch is the right way forward, I think from
a conservative standpoint we need to ship with the checks enabled, but allow
applications like Ruby to do "#define _STRICT_FD_SIZE_CHECK 0", instead of
completely disabling _FORTIFY_SOURCE as they do today e.g. 
#undef _FORTIFY_SOURCE
#undef __USE_FORTIFY_LEVEL
#define __USE_FORTIFY_LEVEL 0

(2) Associate the name of the macro with _FORTIFY_SOURCE.

The name "_STRICT_FD_SIZE_CHECK" does not imply it has anything to do
with _FORTIFY_SOURCE.

Can we name the macro something related? e.g. "_FORTIFY_SOURCE_STRICT_FD_SETSIZE" ?

I don't really care what it is called, but I want to be immediately recognizable
as a _FORTIFY_SOURCE option.

(3) Update patches.

Please either discuss (1) and (2), or post updated patches.

If a patch is not impacted by (1) or (2), then ping the patch
and I'll review it.
 
Cheers,
Carlos.