Network socket not set to nonblocking mode despite using nonblocking API (#2315) · Issues · Xiph.Org / Icecast-libshout

Network socket not set to nonblocking mode despite using nonblocking API

0ac7ed9e introduced a bug which causes network sockets not to be set into nonblocking mode when requested with shout_set_nonblocking(s, 1). This can be demonstrated simply by tracing an example nonblocking app.

Bad (current master HEAD):

$ strace -f -e fcntl,fcntl64,socket,connect ./nonblocking
(...)
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = 0

Good (v2.4.3):

(...)
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
fcntl(4, F_SETFL, O_RDONLY|O_NONBLOCK)  = 0
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now i   n progress)

In the first case fcntl is never called and the socket stays in the default (blocking) mode.

This is due to the following condition in shout_connection_connect:

    if (con->nonblocking == SHOUT_BLOCKING_DEFAULT)
        shout_connection_set_nonblocking(con, shout_get_nonblocking(shout));

But as con->nonblocking is never initialized to this value, it's effectively if(0).

Adding a simple:

    con->nonblocking = SHOUT_BLOCKING_DEFAULT;

to shout_connection_new fixes the problem. But this turns the above condition into if(1), so is it really needed? It was not there before. What's the idea behind this?

https://gitlab.xiph.org/xiph/icecast-libshout/commit/0ac7ed9e84c3871d4427acc1ce59dca5e4af21ef introduced a bug which causes network sockets not to be set into nonblocking mode when requested with `shout_set_nonblocking(s, 1)`. This can be demonstrated simply by tracing an example `nonblocking` app.

Bad (current master HEAD):

```
$ strace -f -e fcntl,fcntl64,socket,connect ./nonblocking
(...)
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
```

Good (v2.4.3):

```
(...)
socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 4
fcntl(4, F_SETFL, O_RDONLY|O_NONBLOCK)  = 0
connect(4, {sa_family=AF_INET, sin_port=htons(8000), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now i   n progress)
```

In the first case `fcntl` is never called and the socket stays in the default (blocking) mode.

This is due to the following condition in `shout_connection_connect`:

```C
    if (con->nonblocking == SHOUT_BLOCKING_DEFAULT)
        shout_connection_set_nonblocking(con, shout_get_nonblocking(shout));
```

But as `con->nonblocking` is never initialized to this value, it's effectively `if(0)`.

Adding a simple:

```C
    con->nonblocking = SHOUT_BLOCKING_DEFAULT;
```

to `shout_connection_new` fixes the problem. But this turns the above condition into `if(1)`, so is it really needed? It was not there before. What's the idea behind this?