signal to sigaction conversion

I'm having trouble converting a server from using signal() to
sigaction().

The original client-server pair works fine.

But the sigaction() version seems to create spurious child processes every so often after another child has terminated.

What have I missed?

Client Code:

Code:

#include <sys/socket.h> #include <sys/types.h> #include <netdb.h> #include <unistd.h> #include <arpa/inet.h> #include <stdlib.h> #include <stdio.h> #include <string.h> #include <sys/wait.h> /* client */ void child_func(int childnum); int main(int argc, char *argv[]) { int nchildren = 1; int pid; int x; if (argc > 1) { nchildren = atoi(argv[1]); } for (x = 0; x < nchildren; x++) { if ((pid = fork()) == 0) { child_func(x + 1); exit(0); } } wait(NULL); return 0; } void child_func(int childnum) { int sock; struct sockaddr_in sAddr; char buffer[25]; memset((void *) &sAddr, 0, sizeof(struct sockaddr_in)); sAddr.sin_family = AF_INET; sAddr.sin_addr.s_addr = INADDR_ANY; sAddr.sin_port = 0; sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); bind(sock, (const struct sockaddr *) &sAddr, sizeof(sAddr)); sAddr.sin_addr.s_addr = inet_addr("127.0.0.1"); sAddr.sin_port = htons(1972); if (connect(sock, (const struct sockaddr *) &sAddr, sizeof(sAddr)) != 0) { perror("client"); return; } snprintf(buffer, 128, "data from client #%i.", childnum); sleep(1); printf("child #%i sent %i chars\n", childnum, send(sock, buffer, strlen(buffer), 0)); sleep(1); printf("child #%i received %i chars\n", childnum, recv(sock, buffer, 25, 0)); sleep(1); close(sock); }

Original Server Code:

Code:

#include <stdio.h> #include <stdlib.h> #include <sys/ioctl.h> #include <sys/types.h> #include <sys/socket.h> #include <netinet/in.h> #include <sys/wait.h> #include <signal.h> #include <netdb.h> #include <unistd.h> /* original server */ void sigchld_handler(int signo) { while (waitpid(-1, NULL, WNOHANG) > 0); } int main(void) { struct sockaddr_in sAddr; int listensock; int newsock; char buffer[25]; int result; int nread; int pid; int val; listensock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); val = 1; result = setsockopt(listensock, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)); if (result < 0) { perror("server2"); return 0; } sAddr.sin_family = AF_INET; sAddr.sin_port = htons(1972); sAddr.sin_addr.s_addr = INADDR_ANY; result = bind(listensock, (struct sockaddr *) &sAddr, sizeof(sAddr)); if (result < 0) { perror("server2"); return 0; } result = listen(listensock, 5); if (result < 0) { perror("server2"); return 0; } signal(SIGCHLD, sigchld_handler); while (1) { newsock = accept(listensock, NULL, NULL); if ((pid = fork()) == 0) { printf("child process %i created.\n", getpid()); close(listensock); nread = recv(newsock, buffer, 25, 0); buffer[nread] = '\0'; printf("%s\n", buffer); send(newsock, buffer, nread, 0); close(newsock); printf("child process %i finished.\n", getpid()); exit(0); } close(newsock); } }

Sigaction Server Code:

Code:

#include <stdio.h> #include <stdlib.h> #include <sys/ioctl.h> #include <sys/types.h> #include <sys/socket.h> #include <netinet/in.h> #include <sys/wait.h> #include <signal.h> #include <netdb.h> #include <unistd.h> #include <string.h> /* sigaction server */ void sigchld_handler(int signo); int main(void) { struct sockaddr_in sAddr = {0}; struct sigaction san; int listensock; int newsock; char buffer[25]; int result; int nread; int pid; int val; memset(&san,0,sizeof(san)); san.sa_handler = sigchld_handler; listensock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); val = 1; result = setsockopt(listensock, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val)); if (result < 0) { perror("server2"); return 0; } sAddr.sin_family = AF_INET; sAddr.sin_port = htons(1972); sAddr.sin_addr.s_addr = INADDR_ANY; result = bind(listensock, (struct sockaddr *) &sAddr, sizeof(sAddr)); if (result < 0) { perror("server2"); return 0; } result = listen(listensock, 5); if (result < 0) { perror("server2"); return 0; } sigaction(SIGCHLD,&san,NULL); while (1) { newsock = accept(listensock, NULL, NULL); if ((pid = fork()) == 0) { printf("child process %i created.\n", getpid()); close(listensock); nread = recv(newsock, buffer, 25, 0); buffer[nread] = '\0'; printf("%s\n", buffer); send(newsock, buffer, nread, 0); close(newsock); printf("child process %i finished.\n", getpid()); exit(0); } close(newsock); } } void sigchld_handler(int signo) { while (waitpid(-1, NULL, WNOHANG) > 0); }

I can't see anything specific to the sigaction-using server as opposed to the signal-using server that would cause the difference. However:

In the client:

* You aren't handling fork errors (returns -1).

* You're only waiting for the first child. Try

Code:

for (x = 0; x < nchildren; x++) if (wait(NULL) == -1) // error

* In snprintf, you pass 128 as the size of the buffer, but it's only 25.

In both servers:

* If you want to receive a max of 25 bytes and then zero-terminate it you need to make your buffer 26 bytes.

* Why are you using the WNOHANG option in waitpid and then spinning? It's far more efficient to pass in 0 and let the system function do the waiting. Also, you should be checking for a possible error (-1) return.

I suggest you make the buffer a little bigger; it's rather a tight fit.

oogabooga has lots of good advice you should follow

First off, this code is dangerous:

Code:

nread = recv(newsock, buffer, 25, 0); buffer[nread] = '\0';

If recv returns -1 there, you have a buffer underrun. If it returns 25 (all the data you asked for), you have a buffer overrun. Either way, you're accessing memory you don't own, which results in undefined behavior, allowing anything to happen. This is not your problem, but it is related.

It may sound mean, but the root of your problem is sloppy code that just assumes certain functions work. Always make sure you check the return value of all the functions you use (sigaction, accept, send, recv, etc). Print out a useful error message (perror or errno+strerror) and act appropriately (exit, continue, etc). Read the man pages thoroughly for every function you use (sections 2 and 3 of the man pages), and for general info (section 7). Read about what causes them to fail, what they return when they fail and what to do about it. If accept fails and returns -1 (perhaps because it was interrupted by a signal), you still fork a child process and pretend everything is OK, even though there is no client to deal with.

There is a lot to consider with code like this, like how fork affects pending signals, signal masks, etc. Also, note that signal() can behave differently depending on your compiler options or whether certain feature test macros are defined. Lastly, note that signal() doesn't block the delivery of the same signal (i.e. receiving more SIGCHLD signals before you finish processing the first one), while sigaction does makes the subsequent SIGCHLD signals wait to be processed. I think that last one is related to your problem, which sounds like a timing issue.

Because signal() lets all the SIGCHLD signals in at any time, they all happen to be delivered at "safe" times, when you're not interrupting a signal call, i.e. not in accept. Thus, accept is always successful, or blocking waiting for a new client to connect. With sigaction(), however, all those child processes that terminate at roughly the same time leave a big backlog of signals for sigchld_handler to deal with. Some of those signals are delivered while you're in the middle of an accept call, and the accept fails with a return of -1 and errno of EINTR, meaning you need to retry.

Ok I'll check your suggestions out.

The code btw is from "The Definitive Guide to Linux Network Programming."

But I read elsewhere that signal() was deprecated and sigaction() should be used in its place.

Quote:

Originally Posted by gemera

The code btw is from "The Definitive Guide to Linux Network Programming."

Then apologies if it was somebody else's sloppy code. Sometimes authors omit error checking because it would use up too much paper if they were that thorough. Still, adding complete error checking should be the first thing you do when something goes wrong.

Quote:

But I read elsewhere that signal() was deprecated and sigaction() should be used in its place.

It has. If the author chose to use signal() instead of sigaction(), they may not have been aware of sigaction (which may have been fairly new when the book was written in 2004 -- 8 years ago), or they just couldn't be bothered to keep up with the changes in Linux. Either way, you probably ought to investigate other books/tutorials. For networking and IPC stuff, I like Beej's guides (Beej's Web Page). Also, I'm a pretty big fan of "The Linux Programming Interface" by Michael Kerrisk. It covers nearly all of Linux and is much more current (2010).

To be honest I've been in two minds about the book.The code examples are full of errors.
But apart from that it's a fairly good beginner text and slightly simplified because it's only dealing with IPv4.

Ok, after much hair-pulling I eventually found the solution.

Sigaction() needs the SA_RESTART flag set for blocking/slow functions like accept().

For some reason the info isn't widely available on-line but the full story can be found here.

"When a system call is slow and a signal arrives while it was blocked, waiting for something,
the call is aborted and returns -EINTR, so that the library function will return -1 and set
errno to EINTR. Just before the system call returns, the user program's signal handler is called.

(So, what is "slow"? Mostly those calls that can block forever waiting for external
events; read and write to terminal devices, but not read and write to disk devices,
wait, pause.)

This means that a system call can return an error while nothing was wrong. Usually one
will want to redo the system call. That can be automated by installing the signal handler
using a call to sigaction with the SA_RESTART flag set. The effect is that upon an interrupt
the system call is aborted, the user program's signal handler is called, and afterwards the
system call is restarted from the beginning."

So the only problem left is the facial twitch I've developed in the process.:cool:

Quote:

Originally Posted by gemera

Ok, after much hair-pulling I eventually found the solution.

Sigaction() needs the SA_RESTART flag set for blocking/slow functions like accept().

For some reason the info isn't widely available on-line but the full story can be found here.

"When a system call is slow and a signal arrives while it was blocked, waiting for something,
the call is aborted and returns -EINTR, so that the library function will return -1 and set
errno to EINTR. Just before the system call returns, the user program's signal handler is called.

(So, what is "slow"? Mostly those calls that can block forever waiting for external
events; read and write to terminal devices, but not read and write to disk devices,
wait, pause.)

This means that a system call can return an error while nothing was wrong. Usually one
will want to redo the system call. That can be automated by installing the signal handler
using a call to sigaction with the SA_RESTART flag set. The effect is that upon an interrupt
the system call is aborted, the user program's signal handler is called, and afterwards the
system call is restarted from the beginning."

So the only problem left is the facial twitch I've developed in the process.:cool:

That info is in the man pages I told you to read (which are easily found on line). If you plan on doing much development on Linux, then you should make a habit of reading them.

The elucidation of the problem was not in the sigaction() manual but at that link.

Quote:

Originally Posted by gemera

The elucidation of the problem was not in the sigaction() manual but at that link.

True, not in the sigaction man page(), but I also suggested you look at the section 7 of the man pages, and the sigaction man page refers you to the signal(7) man page, which has that exact info.

The fact that system calls can be interrupted by signals is one of those basic UNIX things, not a funny corner case. Unfortunately, this fact is poorly documented because it is hard to say which manual page it should be documented by. In defense of the man page writers, the man pages of all system calls do indicate that those calls may return EINTR if interrupted by a signal.

So, sigaction()-gate still has some traction,then! :cool:

@brewbuck

Totally agree - we are talking about mainstream functionality here. The entry
for SA_RESTART in the sigaction() manual should be in red or something to
emphasise it's importance.

@anduril

Appreciate the help you have given but think ultimately your criticism
seems to boil down to "you found the solution on the wrong web-page".

But I still got it, and anyone in the same position will be able to
easily google for the solution in future.

And btw, I felt OBLIGED to give the solution since I had been given the
help.<martyric exasperated look to the heavens>

I had looked at the SA_RESTART flag entry in the sigaction() manual and
dismissed it as a 'funny corner case'. Thus my reference to 'lucidity'.

So from my POV i'm clear on why I found the info hard to find.

But I understand your POV is different.

Quote:

Originally Posted by gemera

So, sigaction()-gate still has some traction,then! :cool:

@brewbuck

Totally agree - we are talking about mainstream functionality here. The entry
for SA_RESTART in the sigaction() manual should be in red or something to
emphasise it's importance.

The problem with SA_RESTART is that it unconditionally restarts the interrupted system call -- there may be times you do not actually want to do that. The general solution is to check for EINTR whenever calling such a function, so that your code has the choice whether to call the function again. If you choose to use SA_RESTART, be aware that you are giving up flexibility by doing so.

Also, SA_RESTART does not apply to certain system calls. To avoid having to memorize the semantics of each one, it is safe to assume that any system call might return EINTR, and check for this.

It might seem like a pain, but it isn't difficult to write wrappers for those syscalls you want to use with automatic restarting. Example:

Code:

int restartable_open(args) { int fd = open(args); while (fd < 0) { if (errno != EINTR) return -1; fd = open(args); } return fd; }

Then call restartable_open() instead of open(). Any error other than EINTR will be reported back as usual, but EINTR will cause it to automatically try again until it's not interrupted.