Discussion:
A possible bug with non-blocking sockets and SIGIO
(too old to reply)
Dmitry Matveev
2011-02-17 07:58:10 UTC
Permalink
Hello,

I have discovered a bug (at least it looks like a bug) in NetBSD's
socket layer.
I use NetBSD 5.0.2, i386 port. Please see the detailed info below (I
have to say
that I am new to NetBSD and to kernel development as well, so there
could be some
wrong assumptions in the text -- I will be happy if someone will
correct me).


Background information
----------------------

Suppose there is a single-threaded server. It uses SIGIO to maintain
incoming
connections and data from clients.

When the process receives SIGIO:
* if there are pending connections on the server socket, the process
calls
accept(2) and then adds an accepted client socket to a list.
* if there is any data available on any client socket (it is
determined via
poll(2)), server processes this data.


Problem description
-------------------

Server process does not receive SIGIO on incoming data on the client
socket.


Problem analysis
----------------

A process will receive SIGIO on incoming data if the owner PID and
O_ASYNC
option are set for the socket. The debugging has shown that the client
socket
does not have SB_ASYNC bit set in its so_rcv.sb_flags, so SIGIO is not
emitted
in sowakeup().

However the process sets the required options this way:

int oldflags = fcntl (sock, F_GETFL, 0);

if (!(oldflags & O_ASYNC)) {
if (-1 == fcntl (sock, F_SETFL, oldflags |= O_ASYNC)) {
exit (1);
}
}

i.e. if there is no O_ASYNC, set it. But the issue is that *there was
O_ASYNC in
the `oldflags`*! So we get in the situation when we have O_ASYNC but do
not have
SB_ASYNC.

How it could happen? sys_fcntl() returns (fp->f_flag - 1) value to a
process
when it asks socket flags via fcntl with F_GETFL.

The value of `fp->f_flag` for client's socket is copied from server's
`fp->f_flag` in do_sys_accept(). Note that server socket is
non-blocking and its
flags contain all the necessary bits. But in sonewconn():

so->so_rcv.sb_flags |= head->so_rcv.sb_flags & SB_AUTOSIZE;

This code sets SB_AUTOSIZE bit at client's `so_rcv.sb_flags`, if this
bit is set
at server's flags. Otherwise the value is 0. Here we lost all other
flags,
including SB_ASYNC. I think that it is the bug.


Possible solution
-----------------

Since do_sys_accept() copies `f_flag` value from server's fp to
client's fp and
there is many data copied from server socket to client socket in
sonewconn(),
I think it would be right to do

so->so_rcv.sb_flags = head->so_rcv.sb_flags;
so->so_snd.sb_flags = head->so_snd.sb_flags;

instead of

so->so_rcv.sb_flags |= head->so_rcv.sb_flags & SB_AUTOSIZE;
so->so_snd.sb_flags |= head->so_snd.sb_flags & SB_AUTOSIZE;

in the sonewconn(). In this case we get socket's `sb_flags` and it's
fp's f_flag
in sync.


Tests
-----

A patched kernel seems to work well. I have tested it with opensshd and
GNU
Smalltalk's Swazoo application server.

A simple test application is also attached. It waits for an incoming
connection
on port 8000, then waits for incoming data from the client and then
terminates.
It can be built in two modes: with blocking and non-blocking server
socket.

Blocking version: gcc -o test_sync onc.c
Non-blocking version: gcc -o test_async onc.c -DV_ASYNC

To perform a test, start an appication, i.e.:

$ test_async &

connect with telnet to it:

$ telnet localhost 8000

type something and hit Enter.

Blocking version works fine without a patch, because initally server fp
flags do
not contain O_ASYNC and application is able to set it.

Non-blocking version works correctly only with the patched kernel.

Both versions work fine on GNU/Linux (but with SIGPOLL instead of
SIGIO).


--------------
Best regards,
Dmitry Matveev
Matthias Scheler
2011-03-06 22:15:52 UTC
Permalink
Post by Dmitry Matveev
I have discovered a bug (at least it looks like a bug) in NetBSD's
socket layer.
I disagree, see below.
Post by Dmitry Matveev
Background information
----------------------
Suppose there is a single-threaded server. It uses SIGIO to maintain
incoming connections and data from clients.
Using SIGIO is bad idea in general. Please use poll(2) or
kqueue(2) + kevent(2) instead.
Post by Dmitry Matveev
Server process does not receive SIGIO on incoming data on the client
socket.
Because you never setup the acceptor socket to do so.

Here are my comment about your source code:

void incoming_data (int i) {
if (i != SIGIO)
exit (0);

It is not safe to use exit(3) from a signal handler, please use
_exit(2) instead.

g_client = accept_client (g_server);
CLOG ("client accepted");

Here is your problem. You newer setup asynchronous I/O on this socket.
You need to use fcntl(2) to set O_ASYNC just as you do on the
acceptor socket.

Kind regards
--
Matthias Scheler http://zhadum.org.uk/

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Matthias Scheler
2011-03-06 22:19:18 UTC
Permalink
Post by Matthias Scheler
Post by Dmitry Matveev
I have discovered a bug (at least it looks like a bug) in NetBSD's
socket layer.
I disagree, see below.
Post by Dmitry Matveev
Background information
----------------------
Suppose there is a single-threaded server. It uses SIGIO to maintain
incoming connections and data from clients.
Using SIGIO is bad idea in general. Please use poll(2) or
kqueue(2) + kevent(2) instead.
Post by Dmitry Matveev
Server process does not receive SIGIO on incoming data on the client
socket.
Because you never setup the acceptor socket to do so.
void incoming_data (int i) {
if (i != SIGIO)
exit (0);
It is not safe to use exit(3) from a signal handler, please use
_exit(2) instead.
g_client = accept_client (g_server);
CLOG ("client accepted");
Here is your problem. You newer setup asynchronous I/O on this socket.
You need to use fcntl(2) to set O_ASYNC just as you do on the
acceptor socket.
I've looked at your source again and you do call fcntl(2) on the
client socket. The problem is probably a race condition. Data
has arrived before your process goes calls wait(). The conclusion
is not to use SIGIO.

Kind regards
--
Matthias Scheler http://zhadum.org.uk/

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
der Mouse
2011-03-07 03:48:27 UTC
Permalink
[...possible SIGIO issue...]
The analysis you give has some validity, I think.

I tried onc.c on 1.4T and 4.0.1 (though with 1.4T I had to hack on
CLOG()'s syntax slightly). 1.4T works correctly with or without
-DV_ASYNC; 4.0.1 works only without, same as you see on 5.0.2. I tried
a 5.1 machine I have guest access to and see the same failure as 4.0.1.

onc.c does indeed have a race condition; given the interface design of
SIGIO, there is an unavoidable race between setting up SIGIO on a new
connection and data arriving. Real servers using SIGIO need to take
care to check, after setting up SIGIO, for any data that may have
arrived before SIGIO was set up. While onc.c does not do this, using
the given test method, this does not matter, because the delay
introduced by having a human initiate the test connection allows the
server plenty of time to establish SIGIO before the data arrives.
The conclusion is not to use SIGIO.
If NetBSD no longer supports use of SIGIO, NetBSD should stop
pretending otherwise: NetBSD should remove the interfaces that appear
to support it (O_ASYNC, SIGIO, and the associated documentation), to
prevent this kind of confusion in the future.

If, on the other hand, NetBSD does still support SIGIO, this kind of
response is of negative utility.

I would much prefer to see the bug fixed. I don't think Dmitry's fix
is right (see below), but I am convinced there is a real bug here -
unless, of course, NetBSD no longer supports SIGIO, which would be a
pretty crippling regression (and in which case this is still a bug; the
relevant interfaces and their documentation should be removed).

I'm not sure whether O_ASYNC is supposed to be inherited from the
accepting socket to the accepted socket. If it is, the relevant
process group setting needs to be copied as well; if not, the flags
copied need to have, at least, O_ASYNC deleted. Adding additional
logging, I find that 1.4T does not copy O_ASYNC from the parent socket
to the child, so I think the correct change is probably to return to
the historical behaviour. This is why I think Dmitry's change is not
the best: it, loosely put, copies O_ASYNC from socket to socket as well
as from file descriptor to file descriptor, and I think it shouldn't be
copied.

/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTML ***@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Dennis Ferguson
2011-03-07 04:26:56 UTC
Permalink
Post by der Mouse
The conclusion is not to use SIGIO.
If NetBSD no longer supports use of SIGIO, NetBSD should stop
pretending otherwise: NetBSD should remove the interfaces that appear
to support it (O_ASYNC, SIGIO, and the associated documentation), to
prevent this kind of confusion in the future.
If, on the other hand, NetBSD does still support SIGIO, this kind of
response is of negative utility.
I agree with this. A canonical "good" use of SIGIO is in the NTP
daemon, where it is about the only commonly available mechanism to
measure packet arrival times that has a hope of not degrading under
increasing server load (== more time not sitting in poll(), hence
bigger delays between the actual arrival of a packet at the machine
and the taking of a time stamp). If use of SIGIO is to be deprecated
and the facility allowed to bit rot there should be some consideration
of what to replace it with in situations where SIGIO appears to be the
best game in town.

Dennis Ferguson



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Dmitry Matveev
2011-03-07 08:47:51 UTC
Permalink
Hi all,

On Sun, 6 Mar 2011 22:48:27 -0500 (EST), der Mouse
Post by der Mouse
onc.c does indeed have a race condition; given the interface design of
SIGIO, there is an unavoidable race between setting up SIGIO on a new
connection and data arriving. Real servers using SIGIO need to take
care to check, after setting up SIGIO, for any data that may have
arrived before SIGIO was set up.
Sure. Actually, GNU Smalltalk sockets are made in this way. The
implementation relies on SIGIO and carefully checks the conditions
you've mentioned with poll(2). Here is a bit more details:
http://dmitrymatveev.co.uk/blog?id=3
Post by der Mouse
While onc.c does not do this, using
the given test method, this does not matter, because the delay
introduced by having a human initiate the test connection allows the
server plenty of time to establish SIGIO before the data arrives.
Yes, I have decided to not to complicate the example because of it.
Post by der Mouse
I'm not sure whether O_ASYNC is supposed to be inherited from the
accepting socket to the accepted socket. If it is, the relevant
process group setting needs to be copied as well; if not, the flags
copied need to have, at least, O_ASYNC deleted. Adding additional
logging, I find that 1.4T does not copy O_ASYNC from the parent socket
to the child, so I think the correct change is probably to return to
the historical behaviour. This is why I think Dmitry's change is not
the best: it, loosely put, copies O_ASYNC from socket to socket as well
as from file descriptor to file descriptor, and I think it shouldn't be
copied.
As far as I know, the general rule to receive SIGIOs is to
1. Set O_ASYNC on a descriptor;
2. Specify a PID or GID there.
So I think it would be correct behavior for a process to set O_ASYNC
and F_SETOWN for an accepted socket explicitly, i.e. the process should
not count on the accepted socket's origins and flags inheritance. In
this case my patch does not break the semantics.

I have modified onc.c a bit - now it does not set owner PID for an
accepted socket. In the logs I see that a server's PID matches the value
obtained by F_GETOWN for this socket, i.e. the relevant process group
setting was copied, isn't it?

But I agree, the second variant (without copying) looks like more
reliable, obvious and strict solution.

Dmitry


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
der Mouse
2011-03-07 09:08:03 UTC
Permalink
Post by Dmitry Matveev
So I think it would be correct behavior for a process to set O_ASYNC
and F_SETOWN for an accepted socket explicitly, i.e. the process
should not count on the accepted socket's origins and flags
inheritance. In this case my patch does not break the semantics.
This is true. The case in which your patch does break things is the
case of code - presumably written to the traditional model, in which
the child socket is not set O_ASYNC even if the parent was - that sets
the parent socket O_ASYNC but expects the child socket to not generate
SIGIO.

I do not actually know whether any such code exists in the wild, but,
given the extreme variety of code that is known to exist, I would be
somewhat surprised if none did.
Post by Dmitry Matveev
I have modified onc.c a bit - now it does not set owner PID for an
accepted socket. In the logs I see that a server's PID matches the
value obtained by F_GETOWN for this socket, i.e. the relevant process
group setting was copied, isn't it?
That would be my inference too: apparently the F_SETOWN-set owner is
getting copied as well.

It might be interesting to try using
fcntl(fd,F_SETFL,fcntl(fd,F_GETFL,0)|O_ASYNC) unconditionally, rather
than doing so only if O_ASYNC is clear in the F_GETFL-returned flags.
While it is in no sense a fix for the problem, if that leads to the
socket being set async as well, it might be a usable workaround for the
short term.

/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTML ***@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Loading...