scylladb / seastar

High performance server-side application framework
http://seastar.io
Apache License 2.0
8.35k stars 1.54k forks source link

stop using deprecated ares_fds() in c-ares #2197

Open tchaikov opened 6 months ago

tchaikov commented 6 months ago

c-ares v1.28.1 deprecated a bunch of functions. for instance, ares_gethostbyname() is deprecated in favor of ares_getaddrinfo()

/home/kefu/dev/scylladb/seastar/src/net/dns.cc:265:9: warning: 'ares_gethostbyname' is deprecated: Use ares_getaddrinfo instead [-Wdeprecated-declarations]
  265 |         ares_gethostbyname(_channel, p->name.c_str(), af, [](void* arg, int status, int timeouts, ::hostent* host) {
      |         ^
/usr/include/ares.h:592:14: note: 'ares_gethostbyname' has been explicitly marked deprecated here
  592 | CARES_EXTERN CARES_DEPRECATED_FOR(ares_getaddrinfo) void ares_gethostbyname(
      |              ^
/usr/include/ares.h:145:22: note: expanded from macro 'CARES_DEPRECATED_FOR'
  145 |       __attribute__((deprecated("Use " #f " instead")))
      |                      ^
/home/kefu/dev/scylladb/seastar/src/net/dns.cc:357:22: warning: 'ares_parse_srv_reply' is deprecated: Use ares_dns_parse instead [-Wdeprecated-declarations]
  357 |             status = ares_parse_srv_reply(buf, len, &start);
      |                      ^
tchaikov commented 6 months ago

see also https://github.com/c-ares/c-ares/commit/5fd3fc3ab34cd2121610bb2b0d0f9ba56d54efd9

dawmd commented 6 months ago

It's worth noting that among the deprecated functions there is ares_fds, which may crash the system (cf. https://c-ares.org/ares_fds.html#NOTES):

The select(2) call which takes the fd_set parameter has significant limitations which can impact modern systems. The limitations can vary from system to system, but in general if the file descriptor value itself is greater than 1024 (not the count but the actual value), this can lead to ares_fds writing out of bounds which will cause a system crash. In modern networking clients, it is not unusual to have file descriptor values above 1024, especially when a library is pulled in as a dependency into a larger project.

c-ares does not attempt to detect this condition to prevent crashes due to both implementation-defined behavior in the OS as well as integrator-controllable tunables which may impact the limits.

bhalevy commented 4 months ago

@tchaikov why does https://github.com/scylladb/seastar/commit/47bfd73e304fd02ed91d7edf6c60d720e767eac5 only Ref this issue rather than Fixes it?

tchaikov commented 4 months ago

@bhalevy see the commit message of 47bfd73e304fd02ed91d7edf6c60d720e767eac5

in which, ares_fds() and ares_process() are not changed yet, because we need to change the way how to poll the events for name resolution. this would need more thoughts before moving forward.

these two APIs were deprecated. but we have not replaced them yet.