Hi,
I have created patches to add socket for reporting new crashes to abrtd. These should fix bug #539566 and its duplicates.
Karel
On 05/04/2010 07:20 PM, 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.
Karel
Patch brings memory leak
==15705== 6 bytes in 1 blocks are possibly lost in loss record 24 of 575 ==15705== at 0x4A0515D: malloc (vg_replace_malloc.c:195) ==15705== by 0x364DC417B2: g_malloc (in /lib64/libglib-2.0.so.0.2200.5) ==15705== by 0x364DC58F9D: g_strdup (in /lib64/libglib-2.0.so.0.2200.5) ==15705== by 0x364DC2FB15: g_io_channel_init (in /lib64/libglib-2.0.so.0.2200.5) ==15705== by 0x364DC6E86F: g_io_channel_unix_new (in /lib64/libglib-2.0.so.0.2200.5) ==15705== by 0x41E8BD: dumpsocket_init (dumpsocket.cpp:504) ==15705== by 0x4188D3: main (Daemon.cpp:849)
-- Nikola
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?
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.
On Mon, 2010-05-17 at 12:05 +0200, Karel Klic wrote:
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).
listen() only limits how many new connections will be kept buffered by kernel, waiting for your accept() call. If you don't limit the number of accept()s you do, then number of connections will not be limited.
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:
- track the last client write time in struct client
- on client connection a timer is enabled (wakeup every N seconds)
- 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?
Sounds ok.
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.
+ static gchar buf[INPUT_BUFFER_SIZE];
Why "static"? Since it's 1k, you can remove "static" and keep it on stack. This way, after use this 1k block won't take up RAM...
Review of new patch:
+ char *path = xasprintf(DEBUG_DUMPS_DIR"/%s-%ld-%u.new", + client->basename, + (long)time(NULL), + client->pid);
You forgot to free it.
strlen("PID="), strlen(".new")
gcc is still stupid and emits strlen() calls for constant strings. sizeof("CONST")-1 is the same as strlen("CONST"), but evaluates at compile time.
From nitpicking department:
+#define HANDLE_INCOMING_STRING(__tag, __field, __max_len, __printable, __allow_slashes) \ + char *__field = try_to_get_string(message, __tag, __max_len, __printable, __allow_slashes); \ + if (__field) \ + { \ + free(client->__field); \ + client->__field = __field; \ + return; \ + }
Names with leading __underscores are reserved for compiler and libc. Use plain names without underscores.
+ Module for a ABRT exception handling hook
"the" ABRT exception handling hook. We don't have many ABRT exception handling hooks, "a" implies we do.
+ * Caller is responsible to call free() to the returned + * pointer.
"to call free() ON the returned pointer"
messsage -- three 's'
-- vda
Hi,
All patches attached, but only 0002-* is updated. Please see comments below.
Karel
On 05/17/2010 02:56 PM, Denys Vlasenko wrote:
The number of accepted sockets was limited by the listen() call. I made the limit more explicit (defined as MAX_CLIENT_COUNT).
listen() only limits how many new connections will be kept buffered by kernel, waiting for your accept() call. If you don't limit the number of accept()s you do, then number of connections will not be limited.
Correct. Fixed.
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:
- track the last client write time in struct client
- on client connection a timer is enabled (wakeup every N seconds)
- 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?
Sounds ok.
Implemented in slightly different way. 1) track the last client write time in struct client 2) on client connection a client-specific timer is enabled (wakeup every N seconds) 3) on wakeup the timer checks the last write time for its socket and if it is more than N seconds in past, the socket is closed 4) when closing the socket, the timer is removed
I rewrote the code to use static array and to read incoming data in chunks.
static gchar buf[INPUT_BUFFER_SIZE];Why "static"? Since it's 1k, you can remove "static" and keep it on stack. This way, after use this 1k block won't take up RAM...
I felt that adding/removing 1 kB on/from stack every time is worse than using heap. Removed `static`.
Review of new patch:
- char *path = xasprintf(DEBUG_DUMPS_DIR"/%s-%ld-%u.new",
client->basename,(long)time(NULL),client->pid);You forgot to free it.
Fixed.
strlen("PID="), strlen(".new") gcc is still stupid and emits strlen() calls for constant strings. sizeof("CONST")-1 is the same as strlen("CONST"), but evaluates at compile time.
I prefer to keep it as strlen(), because of code readability.
When I first read about sizeof("text"), I thought it might be a bug as it was supposed to return the same size as sizeof(pointer) :)
From nitpicking department:
+#define HANDLE_INCOMING_STRING(__tag, __field, __max_len, __printable, __allow_slashes) \
- char *__field = try_to_get_string(message, __tag, __max_len,
__printable, __allow_slashes); \
- if (__field) \
- { \
free(client->__field); \client->__field = __field; \return; \- }
Names with leading __underscores are reserved for compiler and libc. Use plain names without underscores.
True, changed.
- Module for a ABRT exception handling hook
"the" ABRT exception handling hook. We don't have many ABRT exception handling hooks, "a" implies we do.
Fixed.
- Caller is responsible to call free() to the returned
- pointer.
"to call free() ON the returned pointer"
Fixed.
messsage -- three 's'
Fixed.
On Tue, 2010-05-25 at 16:20 +0200, Karel Klic wrote:
Hi,
All patches attached, but only 0002-* is updated. Please see comments below.
I see that you implemented timing out connections! Nice!
+ char *newpath = xstrndup(path, strlen(path) - strlen(".new")); + if (rename(path, newpath) != 0) + strcpy(path, newpath); ... + if (maxCrashReportsSize > 0) + { + check_free_space(maxCrashReportsSize); + trim_debug_dumps(maxCrashReportsSize, path); + }
I think this is a originally my bug, should be ==, not !=: if (rename(path, newpath) == 0) strcpy(path, newpath); because the goal of strcpy is to set path to the name of directory which must not be deleted by trim_debug_dumps(). If rename() == 0 [that is, it succeeded], now the name is "newpath", and we must use it, not "path".
+/* Callback called by glib main loop when a client newly opens ABRT's socket. */ +static gboolean server_socket_cb(GIOChannel *source, + GIOCondition condition, + gpointer data) +{ + if (condition & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) + { + error_msg("dumpsocket: Server socket error"); + return TRUE; + }
Why bother checking this? accept() will deal with errors (by setting errno and returning -1).
+ /* Check the limit for number of simultaneously attached clients. */ + if (client_count >= MAX_CLIENT_COUNT) + { + error_msg("dumpsocket: Too many clients, refusing connection."); + return TRUE; + }
This will leave descriptor in "ready" state, and it will immediately be picked by next loop iteration. The symptom is that when MAX_CLIENT_COUNT is reached, abrtd will start eating 100% CPU. You need to remove the fd from main loop's set of descriptors to watch, and re-add it whenever you see --client_count == MAX_CLIENT_COUNT-1 in client_free().
+ struct sockaddr_un remote; + socklen_t len = sizeof(remote); + int socket = accept(g_io_channel_unix_get_fd(source), (struct sockaddr*)&remote, &len); + if (socket == -1) + { + error_msg("dumpsocket: Server can not accept client"); + return TRUE; + }
(1) Maybe perror_msg? errno value is useful to know in this case... (2) On siccess, do you need to set this new socket "close on exec" too?
+ int socketfd = socket(AF_UNIX, SOCK_STREAM, 0); + if (socketfd == -1) + perror_msg_and_die("dumpsocket: Can't create AF_UNIX socket"); + + /* Set the FD_CLOEXEC flag to the socket. */ + fcntl(socketfd, F_SETFD, fcntl(socketfd, F_GETFD) | FD_CLOEXEC); + + memset(&local, 0, sizeof(local)); + local.sun_family = AF_UNIX; + strcpy(local.sun_path, SOCKET_FILE); + if (bind(socketfd, (struct sockaddr*)&local, sizeof(local)) == -1) + perror_msg_and_die("dumpsocket: Can't bind AF_UNIX socket to '%s'", SOCKET_FILE); + + if (listen(socketfd, MAX_CLIENT_COUNT) != 0) + perror_msg_and_die("dumpsocket: Can't listen on AF_UNIX socket");
We have xfunctions to make it much more compact: ... int socketfd = xsocket(AF_UNIX, SOCK_STREAM, 0); close_on_exec_on(socketfd); memset(&local, 0, sizeof(local)); local.sun_family = AF_UNIX; strcpy(local.sun_path, SOCKET_FILE); xbind(socketfd, (struct sockaddr*)&local, sizeof(local)); xlisten(socketfd, MAX_CLIENT_COUNT);
On Wed, 2010-05-26 at 16:59 +0200, Denys Vlasenko wrote:
On Tue, 2010-05-25 at 16:20 +0200, Karel Klic wrote:
Hi,
All patches attached, but only 0002-* is updated. Please see comments below.
I see that you implemented timing out connections! Nice!
- char *newpath = xstrndup(path, strlen(path) - strlen(".new"));
- if (rename(path, newpath) != 0)
strcpy(path, newpath);...
- if (maxCrashReportsSize > 0)
- {
check_free_space(maxCrashReportsSize);trim_debug_dumps(maxCrashReportsSize, path);- }
I think this is a originally my bug, should be ==, not !=: if (rename(path, newpath) == 0) strcpy(path, newpath); because the goal of strcpy is to set path to the name of directory which must not be deleted by trim_debug_dumps(). If rename() == 0 [that is, it succeeded], now the name is "newpath", and we must use it, not "path".
I fixed it in both locations and committed fix to git.
Hi,
as usual, only patch no. 2 is updated. Please see the comments below.
On 05/26/2010 04:59 PM, Denys Vlasenko wrote:
- char *newpath = xstrndup(path, strlen(path) - strlen(".new"));
- if (rename(path, newpath) != 0)
strcpy(path, newpath);...
- if (maxCrashReportsSize> 0)
- {
check_free_space(maxCrashReportsSize);trim_debug_dumps(maxCrashReportsSize, path);- }
I think this is a originally my bug, should be ==, not !=: if (rename(path, newpath) == 0) strcpy(path, newpath); because the goal of strcpy is to set path to the name of directory which must not be deleted by trim_debug_dumps(). If rename() == 0 [that is, it succeeded], now the name is "newpath", and we must use it, not "path".
Yes, fixed.
+/* Callback called by glib main loop when a client newly opens ABRT's socket. */ +static gboolean server_socket_cb(GIOChannel *source,
GIOCondition condition,gpointer data)+{
- if (condition& (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
- {
error_msg("dumpsocket: Server socket error");return TRUE;- }
Why bother checking this? accept() will deal with errors (by setting errno and returning -1).
Yes, removed also from g_io_add_watch.
- /* Check the limit for number of simultaneously attached clients. */
- if (client_count>= MAX_CLIENT_COUNT)
- {
error_msg("dumpsocket: Too many clients, refusing connection.");return TRUE;- }
This will leave descriptor in "ready" state, and it will immediately be picked by next loop iteration. The symptom is that when MAX_CLIENT_COUNT is reached, abrtd will start eating 100% CPU. You need to remove the fd from main loop's set of descriptors to watch, and re-add it whenever you see --client_count == MAX_CLIENT_COUNT-1 in client_free().
So I added: /* Check the limit for number of simultaneously attached clients. */ if (client_count >= MAX_CLIENT_COUNT) { error_msg("dumpsocket: Too many clients, refusing connection."); g_source_remove(channel_cb_id); channel_cb_id = 0; return TRUE; }
And to client_free(): --client_count; if (!channel_cb_id) { channel_cb_id = g_io_add_watch(channel, (GIOCondition)(G_IO_IN | G_IO_PRI), (GIOFunc)server_socket_cb, NULL); if (!channel_cb_id) perror_msg_and_die("dumpsocket: Can't add socket watch"); }
This should also be valid in all cases.
Here is a snippet that tests it (abrtd -dvvv running): #!/usr/bin/python import socket slist = [] for i in range(0, 50): s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) s.connect("/var/run/abrt.socket") s.sendall("PID=%s\0" % 100) slist.append(s)
- struct sockaddr_un remote;
- socklen_t len = sizeof(remote);
- int socket = accept(g_io_channel_unix_get_fd(source), (struct sockaddr*)&remote,&len);
- if (socket == -1)
- {
error_msg("dumpsocket: Server can not accept client");return TRUE;- }
(1) Maybe perror_msg? errno value is useful to know in this case...
Yes, added.
(2) On siccess, do you need to set this new socket "close on exec" too?
Yes.
- int socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
- if (socketfd == -1)
perror_msg_and_die("dumpsocket: Can't create AF_UNIX socket");- /* Set the FD_CLOEXEC flag to the socket. */
- fcntl(socketfd, F_SETFD, fcntl(socketfd, F_GETFD) | FD_CLOEXEC);
- memset(&local, 0, sizeof(local));
- local.sun_family = AF_UNIX;
- strcpy(local.sun_path, SOCKET_FILE);
- if (bind(socketfd, (struct sockaddr*)&local, sizeof(local)) == -1)
perror_msg_and_die("dumpsocket: Can't bind AF_UNIX socket to '%s'", SOCKET_FILE);- if (listen(socketfd, MAX_CLIENT_COUNT) != 0)
perror_msg_and_die("dumpsocket: Can't listen on AF_UNIX socket");We have xfunctions to make it much more compact: ... int socketfd = xsocket(AF_UNIX, SOCK_STREAM, 0); close_on_exec_on(socketfd); memset(&local, 0, sizeof(local)); local.sun_family = AF_UNIX; strcpy(local.sun_path, SOCKET_FILE); xbind(socketfd, (struct sockaddr*)&local, sizeof(local)); xlisten(socketfd, MAX_CLIENT_COUNT);
Good. I changed the code to use them.
Pushed to git (with Denys's agreement).
On 06/08/2010 03:57 PM, Karel Klic wrote:
Hi,
as usual, only patch no. 2 is updated. Please see the comments below.
On 05/26/2010 04:59 PM, Denys Vlasenko wrote:
- char *newpath = xstrndup(path, strlen(path) - strlen(".new"));
- if (rename(path, newpath) != 0)
- strcpy(path, newpath);
...
- if (maxCrashReportsSize> 0)
- {
- check_free_space(maxCrashReportsSize);
- trim_debug_dumps(maxCrashReportsSize, path);
- }
I think this is a originally my bug, should be ==, not !=: if (rename(path, newpath) == 0) strcpy(path, newpath); because the goal of strcpy is to set path to the name of directory which must not be deleted by trim_debug_dumps(). If rename() == 0 [that is, it succeeded], now the name is "newpath", and we must use it, not "path".
Yes, fixed.
+/* Callback called by glib main loop when a client newly opens ABRT's socket. */ +static gboolean server_socket_cb(GIOChannel *source,
- GIOCondition condition,
- gpointer data)
+{
- if (condition& (G_IO_HUP | G_IO_ERR | G_IO_NVAL))
- {
- error_msg("dumpsocket: Server socket error");
- return TRUE;
- }
Why bother checking this? accept() will deal with errors (by setting errno and returning -1).
Yes, removed also from g_io_add_watch.
- /* Check the limit for number of simultaneously attached clients. */
- if (client_count>= MAX_CLIENT_COUNT)
- {
- error_msg("dumpsocket: Too many clients, refusing connection.");
- return TRUE;
- }
This will leave descriptor in "ready" state, and it will immediately be picked by next loop iteration. The symptom is that when MAX_CLIENT_COUNT is reached, abrtd will start eating 100% CPU. You need to remove the fd from main loop's set of descriptors to watch, and re-add it whenever you see --client_count == MAX_CLIENT_COUNT-1 in client_free().
So I added: /* Check the limit for number of simultaneously attached clients. */ if (client_count >= MAX_CLIENT_COUNT) { error_msg("dumpsocket: Too many clients, refusing connection."); g_source_remove(channel_cb_id); channel_cb_id = 0; return TRUE; }
And to client_free(): --client_count; if (!channel_cb_id) { channel_cb_id = g_io_add_watch(channel, (GIOCondition)(G_IO_IN | G_IO_PRI), (GIOFunc)server_socket_cb, NULL); if (!channel_cb_id) perror_msg_and_die("dumpsocket: Can't add socket watch"); }
This should also be valid in all cases.
Here is a snippet that tests it (abrtd -dvvv running): #!/usr/bin/python import socket slist = [] for i in range(0, 50): s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) s.connect("/var/run/abrt.socket") s.sendall("PID=%s\0" % 100) slist.append(s)
- struct sockaddr_un remote;
- socklen_t len = sizeof(remote);
- int socket = accept(g_io_channel_unix_get_fd(source), (struct
sockaddr*)&remote,&len);
- if (socket == -1)
- {
- error_msg("dumpsocket: Server can not accept client");
- return TRUE;
- }
(1) Maybe perror_msg? errno value is useful to know in this case...
Yes, added.
(2) On siccess, do you need to set this new socket "close on exec" too?
Yes.
- int socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
- if (socketfd == -1)
- perror_msg_and_die("dumpsocket: Can't create AF_UNIX socket");
- /* Set the FD_CLOEXEC flag to the socket. */
- fcntl(socketfd, F_SETFD, fcntl(socketfd, F_GETFD) | FD_CLOEXEC);
- memset(&local, 0, sizeof(local));
- local.sun_family = AF_UNIX;
- strcpy(local.sun_path, SOCKET_FILE);
- if (bind(socketfd, (struct sockaddr*)&local, sizeof(local)) == -1)
- perror_msg_and_die("dumpsocket: Can't bind AF_UNIX socket to '%s'",
SOCKET_FILE);
- if (listen(socketfd, MAX_CLIENT_COUNT) != 0)
- perror_msg_and_die("dumpsocket: Can't listen on AF_UNIX socket");
We have xfunctions to make it much more compact: ... int socketfd = xsocket(AF_UNIX, SOCK_STREAM, 0); close_on_exec_on(socketfd); memset(&local, 0, sizeof(local)); local.sun_family = AF_UNIX; strcpy(local.sun_path, SOCKET_FILE); xbind(socketfd, (struct sockaddr*)&local, sizeof(local)); xlisten(socketfd, MAX_CLIENT_COUNT);
Good. I changed the code to use them.
Crash-catcher mailing list Crash-catcher@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/crash-catcher
crash-catcher@lists.fedorahosted.org
-
Denys Vlasenko -
Karel Klic -
Nikola Pajkovsky