search

strophe / libstrophe

A simple, lightweight C library for writing XMPP clients
http://strophe.im/libstrophe
Other
399 stars 162 forks source link

Memory corruption when using fds > 1023 #166

Open manuelkasper opened 3 years ago

manuelkasper commented 3 years ago

Using libstrophe in a process that has more than 1024 (FD_SETSIZE) open files/sockets leads to memory corruption due to indiscriminate use of FD_SET. The default ulimit of 1024 in most Linux distributions prevents this, but other platforms (e.g. FreeBSD) have higher default limits. Also, one can envision server software that makes use of libstrophe for an upstream XMPP connection while also dealing with lots of other incoming connections in the same process.

Handling fds > FD_SETSIZE seems cumbersome and inefficient with select(). I see two ways to address the memory corruption issue:

  1. Use an alternative like poll().
  2. Or, at a minimum, check the fd numbers before using FD_SET.

I have implemented poll() support in a branch here: https://github.com/manuelkasper/libstrophe/tree/poll (cursory testing done on Linux and FreeBSD; the code still uses select() on Windows, and I have not tested that). If desired, I can make a pull request, but note that in order to avoid dynamic memory allocation for the poll() call, a fixed limit on the number of connections per context (XMPP_MAX_CONNS_PER_CTX) had to be introduced – not sure if this is acceptable.

pasis commented 3 years ago

Thanks for report. Agree, this has to be fixed. I will look into poll/epoll as default implementation on modern systems. Libstrophe has been using select(2) to be more portable to exotic and old systems.

manuelkasper commented 3 years ago

Just a quick side note: in my opinion, poll(2) is perfectly adequate for use in libstrophe (no real need for fancier stuff like epoll or kqueue), and has been supported in Linux, FreeBSD, OpenBSD and Solaris for more than 20 years.

igorvpcleao commented 2 days ago

Hi @pasis, We've been struggling on this as well. Do you believe you folks have an update on this topic coming soon?