This is the new version of my patches for abrt-server allowing more information to be provided from handlers. I've applied or replied to all Denys' comments. My comment below still applies. :)
This set of patches introduce a new version of abrt-server.c that can parse more messages and thus save more information about the crashes handled by Python exception handler (and in future, hopefully, python-meh). I've tested it and it seems to be working well. I know the changes are not ideal as they cannot be applied separately (abrt-server.c is not compilable if they are), but I really do not see any reason why I should remove printable_str() function in one commit and add it back in the following one. If you really need the commits to change the code from one compilable and working state to another, I can squash all these patches to one for you. :)
-- Vratislav Podzimek
--- src/daemon/abrt-server.c | 149 ++++++++++++++-------------------------------- 1 files changed, 46 insertions(+), 103 deletions(-)
diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c index 15f3370..43c6389 100644 --- a/src/daemon/abrt-server.c +++ b/src/daemon/abrt-server.c @@ -77,29 +77,21 @@ static unsigned total_bytes_read = 0; static uid_t client_uid = (uid_t)-1L;
static int pid; -static char *executable; -static char *backtrace; -/* "python", "ruby" etc. */ -static char *analyzer; -/* Directory base name: "pyhook", "ruby" etc. */ -static char *dir_basename; -/* Crash reason. - * Python example: - * "CCMainWindow.py:1:<module>:ZeroDivisionError: integer division or modulo by zero" - */ -static char *reason; -
/* Create a new debug dump from client session. * Caller must ensure that all fields in struct client * are properly filled. */ -static int create_debug_dump() +static int create_debug_dump(GHashTable *problem_info) { /* Create temp directory with the debug dump. This directory is renamed to final directory name after all files have been stored into it. */ + gchar *dir_basename = g_hash_table_lookup(problem_info, "basename"); + GHashTableIter iter; + gpointer gpkey, gpvalue; + char *path = xasprintf("%s/%s-%s-%u.new", g_settings_dump_location, dir_basename, @@ -115,15 +107,17 @@ static int create_debug_dump() } dd_create_basic_files(dd, client_uid);
- dd_save_text(dd, FILENAME_ANALYZER, analyzer); - dd_save_text(dd, FILENAME_EXECUTABLE, executable); - dd_save_text(dd, FILENAME_BACKTRACE, backtrace); - dd_save_text(dd, FILENAME_REASON, reason); - - /* Obtain and save the command line. */ - char *cmdline = get_cmdline(pid); - dd_save_text(dd, FILENAME_CMDLINE, cmdline ? : ""); - free(cmdline); + gpkey = g_hash_table_lookup(problem_info, FILENAME_CMDLINE); + if (!gpkey) + { + /* Obtain and save the command line. */ + char *cmdline = get_cmdline(pid); + if (cmdline) + { + dd_save_text(dd, FILENAME_CMDLINE, cmdline); + free(cmdline); + } + }
/* Store id of the user whose application crashed. */ char uid_str[sizeof(long) * 3 + 2]; @@ -132,6 +126,12 @@ static int create_debug_dump()
dd_save_text(dd, "abrt_version", VERSION);
+ g_hash_table_iter_init(&iter, problem_info); + while (g_hash_table_iter_next(&iter, &gpkey, &gpvalue)) + { + dd_save_text(dd, (gchar *) gpkey, (gchar *) gpvalue); + } + dd_close(dd);
/* Move the completely created debug dump to @@ -141,7 +141,7 @@ static int create_debug_dump() strcpy(path, newpath); free(newpath);
- log("Saved %s crash dump of pid %u to %s", analyzer, pid, path); + log("Saved crash dump of pid %u to %s", pid, path);
/* Trim old crash dumps if necessary */ load_abrt_conf(); @@ -227,87 +227,34 @@ static bool printable_str(const char *str) return true; }
-/* @returns - * Caller is responsible to call free() on the returned - * pointer. - * If NULL is returned, string extraction failed. - */ -static char *try_to_get_string(const char *message, - const char *tag, - size_t max_len, - bool printable, - bool allow_slashes) -{ - if (!starts_with(message, tag)) - return NULL; - - const char *contents = message + strlen(tag); - if ((printable && !printable_str(contents)) - || (!allow_slashes && strchr(contents, '/')) - ) { - error_msg("Received %s contains invalid characters, skipping", tag); - return NULL; - } - - if (strlen(contents) > max_len) - { - error_msg("Received %s too long, trimming to %lu", tag, (long)max_len); - } - - return xstrndup(contents, max_len); -} - /* Handles a message received from client over socket. */ -static void process_message(const char *message) +static void process_message(GHashTable *problem_info, char *message) { -/* @param tag - * The message identifier. Message starting with it - * is handled by this macro. - * @param field - * Member in struct client, which should be filled by - * the field contents. - * @param max_len - * Maximum length of the field in bytes. - * Exceeding bytes are trimmed. - * @param printable - * Whether to limit the field contents to ASCII only. - * @param allow_slashes - * Whether to allow slashes to be a part of input. - */ -#define HANDLE_INCOMING_STRING(tag, field, max_len, printable, allow_slashes) \ -{ \ - char *s = try_to_get_string(message, tag, max_len, printable, allow_slashes); \ - if (s) \ - { \ - free(field); \ - field = s; \ - VERB3 log("Saved %s%s", tag, s); \ - return; \ - } \ -} + gchar *position; + gchar *key, *value;
- HANDLE_INCOMING_STRING("EXECUTABLE=", executable, PATH_MAX, true, true); - HANDLE_INCOMING_STRING("BACKTRACE=", backtrace, MAX_BACKTRACE_SIZE, false, true); - HANDLE_INCOMING_STRING("BASENAME=", dir_basename, 100, true, false); - HANDLE_INCOMING_STRING("ANALYZER=", analyzer, 100, true, true); - HANDLE_INCOMING_STRING("REASON=", reason, 512, false, true); - -#undef HANDLE_INCOMING_STRING + position = strchr(message, '='); + if (position) + { + key = g_ascii_strdown(message, position - message);
- /* PID is not handled as a string, we convert it to pid_t. */ - if (starts_with(message, "PID=")) + position++; + value = xstrndup(position, strlen(position)); + g_hash_table_insert(problem_info, key, value); + } + else { - pid = xatou(message + strlen("PID=")); - if (pid < 1) - /* pid == 0 is error, the lowest PID is 1. */ - error_msg_and_die("Malformed or out-of-range number: '%s'", message + strlen("PID=")); - VERB3 log("Saved PID %u", pid); - return; + error_msg("Invalid message format: '%s'", message); } }
static int perform_http_xact(void) { + /* use free instead of g_free so that we can use xstr* functions from + * libreport/lib/xfuncs.c + */ + GHashTable *problem_info = g_hash_table_new_full(g_str_hash, g_str_equal, + free, free); /* Read header */ char *body_start = NULL; char *messagebuf_data = NULL; @@ -404,7 +351,7 @@ static int perform_http_xact(void) if (len >= messagebuf_len) break; /* messagebuf has at least one NUL - process the line */ - process_message(messagebuf_data); + process_message(problem_info, messagebuf_data); messagebuf_len -= (len + 1); memmove(messagebuf_data, messagebuf_data + len + 1, messagebuf_len); } @@ -427,16 +374,12 @@ static int perform_http_xact(void) error_msg_and_die("Message is too long, aborting"); }
- /* Creates debug dump if all fields were already provided. */ - if (!pid || !backtrace || !executable - || !analyzer || !dir_basename || !reason - ) { - error_msg_and_die("Some data are missing. Aborting"); - } - /* Write out the crash dump. Don't let alarm to interrupt here */ alarm(0); - return create_debug_dump(); + int ret = create_debug_dump(problem_info); + + g_hash_table_destroy(problem_info); + return ret; }
static void dummy_handler(int sig_unused) {}
--- src/daemon/abrt-server.c | 51 ++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c index 43c6389..c0dd830 100644 --- a/src/daemon/abrt-server.c +++ b/src/daemon/abrt-server.c @@ -217,14 +217,46 @@ static int delete_path(const char *dump_dir_name) }
/* Checks if a string contains only printable characters. */ -static bool printable_str(const char *str) +static gboolean printable_str(const char *str) { do { if ((unsigned char)(*str) < ' ' || *str == 0x7f) - return false; + return FALSE; str++; } while (*str); - return true; + return TRUE; +} + +static gboolean is_correct_filename (const char *value) +{ + return printable_str(value) && !strchr(value, '/') && !strchr(value, '.'); +} + +static gboolean key_value_ok(gchar *key, gchar *value) +{ + char *i; + + /* check key, it has to be valid filename and will end up in the + * bugzilla */ + for (i = key; *i != 0; i++) + { + if (!isalpha(*i) && (*i != '-') && (*i != '_') && (*i != ' ')) + return FALSE; + } + + /* check value of 'basename', it has to be valid non-hidden directory + * name */ + if (strcmp(key, "basename") == 0) + { + if (!is_correct_filename(value)) + { + error_msg("Value of 'basename' (%s) is not a valid directory name.", + value); + return FALSE; + } + } + + return TRUE; }
/* Handles a message received from client over socket. */ @@ -240,10 +272,21 @@ static void process_message(GHashTable *problem_info, char *message)
position++; value = xstrndup(position, strlen(position)); - g_hash_table_insert(problem_info, key, value); + if (key_value_ok(key, value)) + if (strcmp(key, FILENAME_UID) == 0) + { + error_msg("Ignoring value of %s, will be determined later.", + FILENAME_UID); + } + else + g_hash_table_insert(problem_info, key, value); + else + /* should use error_msg_and_die() here? */ + error_msg("Invalid key or value format: %s", message); } else { + /* should use error_msg_and_die() here? */ error_msg("Invalid message format: '%s'", message); } }
--- src/daemon/abrt-server.c | 26 ++++++++++++++++++++++++++ 1 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c index c0dd830..ae6d294 100644 --- a/src/daemon/abrt-server.c +++ b/src/daemon/abrt-server.c @@ -291,6 +291,30 @@ static void process_message(GHashTable *problem_info, char *message) } }
+static void die_if_data_is_missing(GHashTable *problem_info) +{ + gboolean have_item, missing_data = FALSE; + gchar **pstring; + static const gchar *const needed[] = {FILENAME_ANALYZER, + FILENAME_BACKTRACE, + FILENAME_EXECUTABLE, + FILENAME_REASON, + "basename", NULL}; + + for (pstring = (gchar**) needed; *pstring; pstring++) + { + have_item = g_hash_table_lookup(problem_info, *pstring) != NULL; + if (!have_item) + { + error_msg("%s is missing.", *pstring); + missing_data = TRUE; + } + } + + if (missing_data) + error_msg_and_die("Some data is missing. Aborting."); +} + static int perform_http_xact(void) { /* use free instead of g_free so that we can use xstr* functions from @@ -419,6 +443,8 @@ static int perform_http_xact(void)
/* Write out the crash dump. Don't let alarm to interrupt here */ alarm(0); + + die_if_data_is_missing(problem_info); int ret = create_debug_dump(problem_info);
g_hash_table_destroy(problem_info);
--- src/daemon/abrt-server.c | 31 ++++++++++++++++++++++++++++--- 1 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c index ae6d294..1076952 100644 --- a/src/daemon/abrt-server.c +++ b/src/daemon/abrt-server.c @@ -76,13 +76,12 @@ static unsigned total_bytes_read = 0;
static uid_t client_uid = (uid_t)-1L;
-static int pid;
/* Create a new debug dump from client session. * Caller must ensure that all fields in struct client * are properly filled. */ -static int create_debug_dump(GHashTable *problem_info) +static int create_debug_dump(GHashTable *problem_info, unsigned pid) { /* Create temp directory with the debug dump. This directory is renamed to final directory name after @@ -315,6 +314,31 @@ static void die_if_data_is_missing(GHashTable *problem_info) error_msg_and_die("Some data is missing. Aborting."); }
+/* + * Takes hash table, looks for key FILENAME_PID and tries to convert its value + * to int. + */ +unsigned convert_pid(GHashTable *problem_info) +{ + long ret; + gchar *pid_str = (gchar *) g_hash_table_lookup(problem_info, FILENAME_PID); + char *err_pos; + int old_errno; + + if (!pid_str) + error_msg_and_die("PID data is missing. Aborting!"); + + old_errno = errno; + errno = 0; + ret = strtol(pid_str, &err_pos, 10); + if (errno || pid_str == err_pos || *err_pos != '\0' + || ret > UINT_MAX || ret < 1) + error_msg_and_die("Malformed or out-of-range PID number: '%s'", pid_str); + errno = old_errno; + + return (unsigned) ret; +} + static int perform_http_xact(void) { /* use free instead of g_free so that we can use xstr* functions from @@ -444,8 +468,9 @@ static int perform_http_xact(void) /* Write out the crash dump. Don't let alarm to interrupt here */ alarm(0);
+ unsigned pid = convert_pid(problem_info); die_if_data_is_missing(problem_info); - int ret = create_debug_dump(problem_info); + int ret = create_debug_dump(problem_info, pid);
g_hash_table_destroy(problem_info); return ret;
--- src/daemon/abrt-server.c | 15 +++++++-------- 1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/src/daemon/abrt-server.c b/src/daemon/abrt-server.c index 1076952..e0f9173 100644 --- a/src/daemon/abrt-server.c +++ b/src/daemon/abrt-server.c @@ -48,28 +48,27 @@ Initializing new dump: open /var/run/abrt.socket
Providing dump data (hook writes to the socket): +MANDATORY ITEMS: -> "PID=" number 0 - PID_MAX (/proc/sys/kernel/pid_max) \0 -> "EXECUTABLE=" - string (maximum length ~MAX_PATH) + string \0 -> "BACKTRACE=" - string (maximum length 1 MB) + string \0 -> "ANALYZER=" - string (maximum length 100 bytes) + string \0 -> "BASENAME=" - string (maximum length 100 bytes, no slashes) + string (no slashes) \0 -> "REASON=" - string (maximum length 512 bytes) + string \0
-Finalizing dump creation: --> "DONE" - \0 +You can send more messages using the same KEY=value format. */
static unsigned total_bytes_read = 0;
crash-catcher@lists.fedorahosted.org