On Tue, 2010-05-04 at 19:20 +0200, Karel Klic wrote:
Hi,
I have created patches to add socket for reporting new crashes to abrtd. These should fix bug #539566 and its duplicates.
I don't see any limit on the number of accepted sockets, or a timeout on the socket which is open but then nothing is ever transferred over it. If you don't what to code it just now, please at least document these dangers in a comment somewhere.
+static int socketfd = -1; +static GIOChannel *channel = NULL;
I remember thatre is a function to extract a unix fd from a channel. Aha! gint g_io_channel_unix_get_fd(GIOChannel *channel)! Therefore you can avoid keeping socketfd in static global variable. Whereever you are using socketfd now, just use this function instead.
+ struct client *client = (struct client*)xmalloc(sizeof(struct client)); + client->uid = uid; + client->messagebuf = g_byte_array_new(); + client->executable = NULL; + client->pid = 0; + client->backtrace = NULL; + client->analyzer = NULL; + client->basename = NULL; + client->reason = NULL; + return client;
Use xzalloc instead - this way NULL/0 assignments can be dropped.
+ char path[PATH_MAX]; + unsigned path_len = snprintf(path, + sizeof(path), + DEBUG_DUMPS_DIR"/%s-%ld-%u.new", + client->basename, + (long)time(NULL), + client->pid); + if (path_len >= sizeof(path)) + { + error_msg("dumpsocket: Dump path too long -> ignoring dump"); + return; + }
PATH_MAX can be 4k. Consider using xasprintf which (1) allocates only as much space as needed, and (2) never fails.
+ HANDLE_INCOMING_STRING("BASENAME=", basename, 100, true);
Here you must sanitize the basename: must not contain slashes. Imagine basename == "../../../../../../etc"
+ /* xatou() cannot be used here, because it would + * kill whole daemon by non-numeric string. + */ + char *endptr; + int old_errno = errno; + errno = 0; + const char *nptr = message + strlen("PID="); + unsigned long number = strtoul(nptr, &endptr, 10); + /* pid == 0 is error, the lowest PID is 1. */ + if (errno || nptr == endptr || *endptr != '\0' || number > UINT_MAX || number == 0) + { + error_msg("dumpsocket: invalid PID received -> ignoring"); + return; + } + errno = old_errno; + client->pid = number; + return;
You do not need to save/restore errno here.
+ if ((socket = accept(socketfd, (struct sockaddr*)&remote, &len)) == -1) + { + error_msg("dumpsocket: Server can not accept client"); + return TRUE; + }
Why make it harder to read? You can do:
socket = accept(socketfd, (struct sockaddr*)&remote, &len); if (socket == -1) ...
+static int set_cloexec_flag(int desc, int value) +{ + int oldflags = fcntl(desc, F_GETFD, 0);
Redundant parameter 0.
+ /* If reading the flags failed, return error indication now. */ + if (oldflags < 0) + return oldflags;
This fails only if desc is not an opened fd - impossible here.
+ /* Set just the flag we want to set. */ + if (value != 0) + oldflags |= FD_CLOEXEC; + else + oldflags &= ~FD_CLOEXEC;
There is only one flag defined, FD_CLOEXEC (== 1). On kernel side, FD_ flags are even packed in bit array, one BIT per fd. Thus, you can be fairly sure no new FD_ flags are going to appear in Linux anytime soon. Thus, you can just use: oldflags = value
+ /* Store modified flag word in the descriptor. */ + return fcntl(desc, F_SETFD, oldflags);
Same.
+} ... + if (set_cloexec_flag(socketfd, 1) != 0) + perror_msg_and_die("dumpsocket: Failed to set CLOEXEC flag.");
Therefore you can just replace all of the above with the single line:
fcntl(desc, F_SETFD, FD_CLOEXEC);
or if you feel paranoid today:
fcntl(desc, F_SETFD, fcntl(desc, F_GETFD) | FD_CLOEXEC);
+ g_byte_array_append(client->messagebuf, buf, len); + for (; loop < client->messagebuf->len; ++loop) + { + if (client->messagebuf->data[loop] != '\0') + continue; + + VERB3 log("dumpsocket: Processing message: %s", + client->messagebuf->data); + + /* Process the complete message. */ + process_message(client, (char*)client->messagebuf->data); + /* Remove the message including the ending \0 */ + g_byte_array_remove_range(client->messagebuf, 0, loop + 1); + loop = 0; + } + g_free(buf);
You can move g_free up, putting it right after g_byte_array_append.
+ # Open ABRT daemon's socket and write data to it. + s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + s.connect(@VAR_RUN@ + "/abrt.socket") + s.sendall("NEW\0") ^^^^^^^^^^^^^^^^^^^^ redundant + s.sendall("PID=%s\0" % pid) + s.sendall("EXECUTABLE=%s\0" % executable) + s.sendall("BACKTRACE=%s\0" % tb) ^^^^^^^^^^^^^^^^^^^^ this one can be big, maybe send it as a last element?