Fix bind local device

[marcovsz]

sizeof(channel->local_dev_name) will always be 32 and cause ares_str_isprint() check failed in default_asetsockopt()

[createyourpersonalaccount]

New code does not store the NUL byte. What bug is this fixing? If you invoke default_asetsockopt with the right number, namely the string length, it will not fail as you claim.

[marcovsz]

If we set channel->local_dev_name to "eth0", sizeof(channel->local_dev_name) is 32, ares_strlen(channel->local_dev_name) is 4, the function calling of default_asetsockopt(sock, ARES_SOCKET_OPT_BIND_DEVICE, "ccmni0", 32, user_data) is not working as expected. It returns -1 after ares_str_isprint() check failed, doesn't run to setsockopt line.

case ARES_SOCKET_OPT_BIND_DEVICE:
if (!ares_str_isprint(val, (size_t)val_size)) {
SET_SOCKERRNO(EINVAL);
return -1;
}
#ifdef SO_BINDTODEVICE
return setsockopt(sock, SOL_SOCKET, SO_BINDTODEVICE, val, val_size);
#else
SET_SOCKERRNO(ENOSYS);
return -1;
#endif

[bradh352]

manpage does say it needs to be a null-terminated string. That said, it doesn't say if the length provided needs to include the NUL character.

That said, the logic here is definitely wrong:

c-ares/src/lib/ares_set_socket_functions.c

Line 291 in a8c0917

if (!ares_str_isprint(val, (size_t)val_size)) {

We should check that the buffer up to the first NUL byte (not to exceed the provided length) is printable.
We probably need an ares_strlen_max() helper (aka strnlen()) to do this.

I think the change in this PR is also valid, however, especially if its been tested to work. But the function called should also be fixed to prevent regressions in the future.

[createyourpersonalaccount]

The old code writes exact amount (32) of bytes. The new code uses strlen without bounds-checking, writing an arbitrary amount of bytes to fixed-width array. There is also an issue of a buffer overflow.

[bradh352]

channel->local_dev_name is a buffer we provide during and copied into during ares_init_options, so its guaranteed to be a valid null-terminated string without overflow.

[marcovsz]

manpage does say it needs to be a null-terminated string. That said, it doesn't say if the length provided needs to include the NUL character.

That said, the logic here is definitely wrong:

c-ares/src/lib/ares_set_socket_functions.c

Line 291 in a8c0917

if (!ares_str_isprint(val, (size_t)val_size)) {

We should check that the buffer up to the first NUL byte (not to exceed the provided length) is printable. We probably need an ares_strlen_max() helper (aka strnlen()) to do this.

I think the change in this PR is also valid, however, especially if its been tested to work. But the function called should also be fixed to prevent regressions in the future.

Of course this change works for me(on an Android Arm platform)