Re: FD_SET and FORTIFY_SOURCE

[KOSAKI Motohiro <kosaki dot motohiro at gmail dot com>]

(3/25/13 1:47 PM), Carlos O'Donell wrote:
> On 03/25/2013 01:24 PM, KOSAKI Motohiro wrote:
>> (3/25/13 1:04 PM), Carlos O'Donell wrote:
>>> On 03/24/2013 04:17 PM, KOSAKI Motohiro wrote:
>>>> 1. only turn on __FD_ELT check when running on hurd.
>>>> 2. only turn on __FD_ELT check when defined some specific macro. (e.g.
>>>> likes darwin,
>>>>     but disable by default)
>>>> 2-2. make FORTIFY_SOURCE variant and check POSIX compliance if enabled.
>>>> 3. provide select_large_fdset() likes solaris. (I strongly don't
>>>> recommend. all application
>>>>     need to modify and recompilation)
>>>>
>>>> What do you think?
>>>
>>> The value of __FD_SETSIZE is part of the ABI and can't be changed.
>>
>> ??
>> We never discuss to change __FD_SETSIZE. A lot of UNIX support >FD_SETSIZE,
>> but didn't change FD_SETSIZE.
> 
> Thank you for the clarification, you want to change the macros to simply
> avoid checking for the size and let the user manage it.
> 
> The same problem still applies. If you pass this set to another library
> compiled with the old macros it will fail.
> 
> How do you avoid that breakage without recompiling all dependent 
> applications?
> 
> Your change would be an API breakage?

It depned on what breakage is. The api was broken by following commit.
So, reverting might be breakage and might be just _fix_.

commit a0f33f996f7986dbf37631a4577f8565b42df29e
Author: Ulrich Drepper <drepper@gmail.com>
Date:   Thu Sep 8 19:48:47 2011 -0400

    Add range checking for FD_SET, FD_CLR, and FD_ISSET


But, you are right. interoperability is also important. So, I'd suggest
to apply following incremental patch w/ my previous one.

diff --git a/debug/fdelt_chk.c b/debug/fdelt_chk.c
index d149476..e14ec6e 100644
--- a/debug/fdelt_chk.c
+++ b/debug/fdelt_chk.c
@@ -21,9 +21,6 @@
 long int
 __fdelt_chk (long int d)
 {
-  if (d < 0 || d >= FD_SETSIZE)
-    __chk_fail ();
-
   return d / __NFDBITS;
 }


I mean, old application still may call __fdelt_chk(), but it doesn't make
program abort. 


>>> Alternatively rewrite using poll/epoll.
>>
>> As far as I know, all of moder scripting language expose select interface to scripting
>> and poll has different semantics against select(2). That makes no sense.
>>
>> One of the way is, to avoid to use FD_SET and other FD_* family likes perl.
>> perf enforce users to make fd bitmap by themself (by using vec()).
>>
>> http://perldoc.perl.org/functions/select.html
>>
>> But, it is extream ugly and useless. I don't think this makes a lot of sense.
> 
> Given that FD_SET that checks for <= 1024 is already in use, 

Who? I really doubt anybody uses current macro meangfullly.


> you can't
> change the interface without interoperability issues between libraries.
> 
> New macro?

New macro doesn't solve anything. New macro mean endorse to recompile. But when
recompiling is acceptable, we can just change and fix current interface. 
FD_SET is already in use from multiple applications w/ howmany(). again we don't
talk about new feature at all. I hope you also skimm debian code search or something else.

I would like to suggest _CHECK_FD_SETSIZE_STRICTLY new macro and enable range check only when
_CHECK_FD_SETSIZE_STRICTLY=1 and _FORTIFY_SOURCE=2. That doesn't break any existing applications.

likes below.



commit ee72234de4308a6bff1c970cd7624ab0da969739
Author: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
Date:   Sun Mar 24 21:55:40 2013 -0400

    [PATCH] enable FD_SETSIZE range check only when defined _CHECK_FD_SIZE_STRICTLY=1
    
    Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@gmail.com>

diff --git a/ChangeLog b/ChangeLog
index e7991c9..ef9d4f3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2013-03-24  KOSAKI Motohiro  <kosaki.motohiro@gmail.com>
+
+	* misc/sys/select.h: Don't enable FD_* argument check
+	when running on Linux.
+
 2013-01-09  Anton Blanchard  <anton@samba.org>
 
 	* sysdeps/unix/sysv/linux/powerpc/sched_getcpu.c: New file.
diff --git a/debug/fdelt_chk.c b/debug/fdelt_chk.c
index d149476..334bde3 100644
--- a/debug/fdelt_chk.c
+++ b/debug/fdelt_chk.c
@@ -17,13 +17,22 @@
 
 #include <sys/select.h>
 
+/* never be removed. this is used from old applications. */
+long int
+__fdelt_chk_nocheck (long int d)
+{
+  return d / __NFDBITS;
+}
+strong_alias (__fdelt_chk_nocheck, __fdelt_chk)
+strong_alias (__fdelt_chk, __fdelt_warn)
+
 
 long int
-__fdelt_chk (long int d)
+__fdelt_check (long int d)
 {
   if (d < 0 || d >= FD_SETSIZE)
     __chk_fail ();
 
   return d / __NFDBITS;
 }
-strong_alias (__fdelt_chk, __fdelt_warn)
+strong_alias (__fdelt_check, __fdelt_warning)
diff --git a/misc/bits/select2.h b/misc/bits/select2.h
index 03558c9..7fe7b66 100644
--- a/misc/bits/select2.h
+++ b/misc/bits/select2.h
@@ -21,8 +21,8 @@
 #endif
 
 /* Helper functions to issue warnings and errors when needed.  */
-extern long int __fdelt_chk (long int __d);
-extern long int __fdelt_warn (long int __d)
+extern long int __fdelt_check (long int __d);
+extern long int __fdelt_warning (long int __d)
   __warnattr ("bit outside of fd_set selected");
 #undef __FD_ELT
 #define	__FD_ELT(d) ¥
@@ -31,5 +31,5 @@ extern long int __fdelt_warn (long int __d)
      (__builtin_constant_p (__d)					    ¥
       ? (0 <= __d && __d < __FD_SETSIZE					    ¥
 	 ? (__d / __NFDBITS)						    ¥
-	 : __fdelt_warn (__d))						    ¥
-      : __fdelt_chk (__d)); })
+	 : __fdelt_warning (__d))					    ¥
+      : __fdelt_check (__d)); })
diff --git a/misc/sys/select.h b/misc/sys/select.h
index 21351fe..b748f61 100644
--- a/misc/sys/select.h
+++ b/misc/sys/select.h
@@ -124,7 +124,7 @@ extern int pselect (int __nfds, fd_set *__restrict __readfds,
 
 
 /* Define some inlines helping to catch common problems.  */
-#if __USE_FORTIFY_LEVEL > 0 && defined __GNUC__
+#if __USE_FORTIFY_LEVEL > 0 && defined __GNUC__ && _CHECK_FD_SIZE_STRICTLY > 0
 # include <bits/select2.h>
 #endif