Hi,
Thank you for the review. The patches are attached, and only 0002-* is updated. Please see comments below.
Karel
On 05/13/2010 10:00 PM, Denys Vlasenko wrote:
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.
The number of accepted sockets was limited by the listen() call. I made the limit more explicit (defined as MAX_CLIENT_COUNT).
Not sure what is the best way how to implement the timeout. Is there any (glib?) trick that can do the timeout for me?
Here is how I would do it: 1) track the last client write time in struct client 2) on client connection a timer is enabled (wakeup every N seconds) 3) on wakeup the timer checks the last write time for every opened socket and if it is more than N seconds in past, the socket is closed 4) if the number of opened sockets is 0, disable the timer
Does someone have better idea?
+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.
Ok.
- 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.
Yes.
- 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.
Yes, I changed it to use xasprintf().
- HANDLE_INCOMING_STRING("BASENAME=", basename, 100, true);
Here you must sanitize the basename: must not contain slashes. Imagine basename == "../../../../../../etc"
Yes, that was serious vulnerability. Thank you for catching that.
/* 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.
Sure.
- 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) ...
Sure. I copied it from the older code.
+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);
Ok. I feel uncomfortable about not checking fcntl return value and errno, but I trust your knowledge in that regard.
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.
I rewrote the code to use static array and to read incoming data in chunks.
- # 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
I removed the NEW command.
- 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?
Yes.