Fix FD_SET macro stack smashing error

[belge-sel]

FD_SET macro is considered insecure, it does not check boundaries and causes segfaults.

This fix does use poll(2), which is considered safer than select(2).

[belge-sel]

FD_SET macro is limited by FD_SETSIZE constant defined in platforms. In most systems it is defined as 1024. This causes libcli to mangle stack bits and cause segfaults in applications when file descriptors bigger than FD_SETSIZE is given as sockfd.

This patch enables poll(2) which is safer and also POSIX (cross-platform) for the library.

[belge-sel]

I avoid changing the code and design drastically and tried to respect the design. But this code can be improved even more. I can provide more detail about the issue (if demanded) and contribute more to make it more robust.

[RobSanders]

There was a previous merge request several years ago to change the select() to poll() or epoll(). As I recall the reason this wasn't done is that at the time select() was more prevalent in some of the platforms using libcli. My understanding is that poll() is much more common now. If poll() is present on all platforms where libcli is used then moving to use it is a good idea, if only to prevent the issue of a file descriptor > 1024. For example, anyone using libcli on a Windows version prior to Vista, or older Mac OS X platforms.
It may be better to wrap this in a #LIBCLI_USE_POLL guard, and let the user tweak as they need. Without making serious changes to the libcli build process I do not think there is a cross-platform way of autodetecting the presence of the poll.h header in order to use that.

[belge-sel]

Yeah, that seems fine. I will make another patch with #LIBCLI_USE_POOL guard.

[RobSanders]

When you do this, please set it up so the default is to keep the current select() call. Let the user define LIBCLI_USE_POLL when building (for example - make -D LIBCLI_USE_POLL) to override the behavior.
My reasoning is that with libcli being a LGPL code, any source code change would require releasing that modified code (and possibly the project using it) as open source. By having a compile time check like this, where the end user is not editing the source code, we avoid that situation. Yeah, this is splitting some hairs - in some ways there isn't much difference between editing the code to set LIBCLI_USE_POLL and passing it in via a -D directive. But the use of the preprocessor define lets us control the build without editing the libcli code base.

[belge-sel]

I edited my commit with requested behavior. It appears Windows is still unable to support poll(2) so it is excluded with define guards.

Users can compile with make CFLAGS=-DLIBCLI_USE_POLL on supported platforms to enable poll(2). It would be nice to have this information available in README.md.

[RobSanders]

Concur, was just looking over your patch. I'll likely merge it in shortly. My primary platform is Linux, so I don't have a Windows platform to test with. Will run a few tests also on my side. I'm currently working on the V 1.10.5 release, which fixes some bugs we found in our app that uses libcli and cleans up some stuff identified in a static code analysis.

[RobSanders]

I was preparing to merge this and realized I don't have a good attribution for you. Also, I may put an additional check fairly early on in cli_loop() if the 'select' call is used to compare it with FD_SETSIZE and punt with an error message if out of bounds. Do you have a preferred email address for attribution in the changelog section? Or are you ok with me using my email and tagging your by your github name in the text of the changelog? I'll likely merge this in the next day or so and then update the code and changelog in my 1_10_5 staging branch.

[belge-sel]

Sweet, checking for errors early is a good idea. Using your email would be OK.

[RobSanders]

Ok, will do under my email, recognizing your github name in the body. Will merge first, then update spec in my branch with attributions.