Skip to main content
added 546 characters in body
Source Link
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

You mentioned in the comments that you used long because that matches the return value of strtol(), which makes sense in a way. However, it is better to store values in the type associated with the intended use later on, as that will avoid implicit casts which might be problematic. It might also be a good idea to parse numbers into a temporary long variable, then check if the value is not too large (for example, a port number should never be larger than 65535) before storing it into the variable of the correct type.

If you are targetting only Linux and/or BSD platforms, consider using err() and warn() from <err.h>, which combine printing a custom message, the result of strerror(errno), and if desired, quiting with a non-zero exit code all in one go. Or you can indeed use quit_with_error_message(), but then make sure you use it consistently.

I see some cases of malloc() not being matched by a corresponding free(). IfIf you accept() a socket, make sure you close() it after you are done with it. Failure to do so will cause resource leaks, and may cause your program to stop after running for a while because it ran out of memory or file descriptors.

If you are targetting only Linux and/or BSD platforms, consider using err() and warn() from <err.h>, which combine printing a custom message, the result of strerror(errno), and if desired, quiting with a non-zero exit code all in one go.

I see some cases of malloc() not being matched by a corresponding free(). If you accept() a socket, make sure you close() it after you are done with it. Failure to do so will cause resource leaks, and may cause your program to stop after running for a while because it ran out of memory or file descriptors.

You mentioned in the comments that you used long because that matches the return value of strtol(), which makes sense in a way. However, it is better to store values in the type associated with the intended use later on, as that will avoid implicit casts which might be problematic. It might also be a good idea to parse numbers into a temporary long variable, then check if the value is not too large (for example, a port number should never be larger than 65535) before storing it into the variable of the correct type.

If you are targetting only Linux and/or BSD platforms, consider using err() and warn() from <err.h>, which combine printing a custom message, the result of strerror(errno), and if desired, quiting with a non-zero exit code all in one go. Or you can indeed use quit_with_error_message(), but then make sure you use it consistently.

If you accept() a socket, make sure you close() it after you are done with it. Failure to do so will cause resource leaks, and may cause your program to stop after running for a while because it ran out of memory or file descriptors.

Source Link
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

Use the appropriate types

You use long and long long in several places. The C standard specifies the relative size between short/int/long/long long, but the exact size depends on the platform, and might be smaller or larger than you think. Make sure you choose the appropriate type. Here is a list of variables whose types should be changed:

  • long port: a TCP port number is only 16 bits. A long is often way too big. Use uint16_t instead.
  • long time_limit, long maxtime: on 64-bit Windows platforms and most 32-bit platforms, storing UNIX time in seconds in a long will cause it to wrap in 2038. Apart from that, it doesn't match the return type of time(). Be consistent and use time_t instead.
  • unsigned long long core_mask: on most platforms this is only 64 bits, but this is not enough if you have an AMD 3990X CPU in your desktop PC, which has 128 logical cores, or if you are running on a multi-processor server which might have even more cores. On Linux, consider using cpu_set_t to store core masks.
  • char server_ip[40]: prefer parsing any textual IP address before storing it in a struct. That avoids any issues with string lengths. 40 is way too much if you only support numeric IPv4 addresses, but if you want to support hostnames, it is too short. It might have been just enough for numeric IPv6 addresses, but it doesn't seem like you support IPv6 in your code. I would use struct sockaddr_storage, which stores both address and port, and supports IPv4 and IPv6.

Simplify zeroing structs

In prepare_submit_message(), you spend a lot of lines of code initializing job. You can simplify this a lot by using designated initializers:

struct Job job = {
    .user_id = geteuid(),
    .group_id = getegid(),
};

All the fields of struct Job that are not explicitly specified will be set to zero, so no more need for = 0 and memset().

Print all error messages to stderr

You are inconsistent in how you are printing error messages. Make sure all of them get printed to stderr. Ideally, usage information printed after an error parsing the command line should also go to stderr.

If you are targetting only Linux and/or BSD platforms, consider using err() and warn() from <err.h>, which combine printing a custom message, the result of strerror(errno), and if desired, quiting with a non-zero exit code all in one go.

Support IPv6

Your code only works for IPv4. We've run out of IPv4 addresses, so you should really make sure your code supports IPv6 as well. It's not much work: ensure you use functions that are independent of IP version to manipulate addresses, such as getaddrinfo() and getnameinfo(). getaddrinfo() will also return the socket type that as appropriate for a given address, so you can pass that to socket().

On Linux at least, if you bind to IPv6, and make sure the socket option IPV6_V6ONLY is set to false, it will also bind to IPv4, so you only need a single listening socket to support incoming connections from both IPv4 and IPv6 addresses. Otherwise, consider creating two listening sockets; one for IPv4 and one for IPv6.

Missing or incorrect error checking

If getcwd() returns NULL, you assume that the only possibe error could be that the working directory name is longer than your buffer. However, there are other possible reasons it can fail. Don't make assumptions, let perror() or err() print the actual error message.

send() and read() can succeed but send or receive less bytes than requested. This is especially likely for TCP sockets. Make sure you handle this scenario and send/receive the remaining bytes if this happens.

Sometimes you don't check return values at all. For example, you don't check the return value of accept().

There are also some functions that programmers typically don't check the return value of, because errors are very unlikely, but even snprintf(), pthread_mutex_lock(), strftime() and so on can return errors. Consider for all functions you call whether it can return an error, and if so what could go wrong if you don't handle errors correctly.

Don't quit due to network errors

Network errors are likely to happen, are not under your control, and temporary network issues can resolve themselves after a while. If something goes wrong when receiving data from a socket or sending data to a socket, don't quit your program, instead just close the socket and wait for the next one.

Clean up properly

I see some cases of malloc() not being matched by a corresponding free(). If you accept() a socket, make sure you close() it after you are done with it. Failure to do so will cause resource leaks, and may cause your program to stop after running for a while because it ran out of memory or file descriptors.

If you call pthread_mutex_init(), make sure you call pthread_mutex_destroy() after you have finished using that mutex. Maybe it's not important since your program is going to exit anyway, but it helps static and runtime analyzers ensure you have no resource leaks.

Denial of service attack

It's easy to prevent anyone from submitting jobs to your job server: just open a TCP connection to its listening socket, and then don't send anything.

Don't sleep unnecessarily

There is a sleep(1) in server(). Why force a one second wait between incoming connections? It does not seem useful, and will only make job submissions take longer than necessary.

The calls to usleep() are also problematic. First, usleep() is a deprecated POSIX function, you should use nanosleep() or clock_nanosleep() instead. Apart from that, don't assume that sleeping for a bit after kill() is enough to ensure that the process you just sent SIGKILL are really killed. In any case, you either wait longer than necessary, or not long enough. Consider using waitpid() without WNOHANG to wait until a process is no longer running.

As for the sleep(1) inside queue(), you do want to sleep if there are no jobs to run, otherwise it would just spin and keep the CPU busy for no good reason. But again, just sleeping a random amount of time is not great, as it puts a limit on the number of jobs you can run in a given amount of time. In this case, use a condition variable that you signal inside server() whenever a new job is submitted, and which you wait for in queue(). For waking up if a job process has exited, you can register a handler for SIGCHILD

Don't call external programs unnecessarily

The setting of CPU affinities is ultimately done over taskset, although I feel I could have implemented that myself aswell, [...]

Starting an external program has lots of issues. Apart from the large amount of code you had to write, the nasty string manipulation you had to do, it is very inefficient. taskset is Linux-specific anyway, so since you don't have to worry about being platform independent, just use sched_setaffinity() to change the core affinity mask of a given process.

Missing mutex for available_cores

Several threads can modify available_cores, but no lock is taken to ensure the value is modified atomically. This means the core mask can potentially become corrupt, and either too many jobs will run simultaneously, or not all cores will be used. A simple solution is to add a mutex for it, or to use one of the existing mutexes to guard this variable.