Signed-off-by: Jakub Filak jfilak@redhat.com --- src/gui-gtk/main.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/gui-gtk/main.c b/src/gui-gtk/main.c index 12b14e9..87c8205 100644 --- a/src/gui-gtk/main.c +++ b/src/gui-gtk/main.c @@ -153,6 +153,19 @@ static void watch_this_dir(const char *dir_name) } }
+/* Returns non 0 if str is NULL or if str consists from white spaces */ +static int is_null_or_mepty_string(const char *str) +{ + if (!str) + return 1; + + while(*str) + if (!isspace(*str++)) + return 0; + + return 1; +} + static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data) { bool use_dbus = (bool)data; @@ -192,15 +205,25 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data time_buf[time_len] = '\0'; }
- const char *not_reportable_reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE); - const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON); + const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE); + if (is_null_or_mepty_string(reason)) /* if problem is NOT not reportable we use reason */ + { + reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON); + if (is_null_or_mepty_string(reason)) /* if we don't have reason we use 'N/A' */ + reason = "N/A"; + }
/* the source of the problem: * - first we try to load component, as we use it on Fedora */ const char *source = problem_data_get_content_or_NULL(pd, FILENAME_COMPONENT); - if (!source) /* if we don't have component, we fallback to executable */ + if (is_null_or_mepty_string(source)) /* if we don't have component, we fallback to executable */ + { source = problem_data_get_content_or_NULL(pd, FILENAME_EXECUTABLE); + if (is_null_or_mepty_string(source)) /* even if we don't have executable, we use 'N/A' */ + source = "N/A"; + } + const char *msg = problem_data_get_content_or_NULL(pd, FILENAME_REPORTED_TO);
GtkListStore *list_store = s_dumps_list_store; @@ -215,7 +238,7 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data gtk_list_store_append(list_store, &iter); gtk_list_store_set(list_store, &iter, COLUMN_SOURCE, source, - COLUMN_REASON, not_reportable_reason ? : reason, + COLUMN_REASON, reason, //OPTION: time format COLUMN_LATEST_CRASH_STR, time_buf, COLUMN_LATEST_CRASH, t,
On 07/31/2012 01:35 PM, Jakub Filak wrote:
Signed-off-by: Jakub Filak jfilak@redhat.com
src/gui-gtk/main.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/gui-gtk/main.c b/src/gui-gtk/main.c index 12b14e9..87c8205 100644 --- a/src/gui-gtk/main.c +++ b/src/gui-gtk/main.c @@ -153,6 +153,19 @@ static void watch_this_dir(const char *dir_name) } }
+/* Returns non 0 if str is NULL or if str consists from white spaces */ +static int is_null_or_mepty_string(const char *str) +{
- if (!str)
return 1;
- while(*str)
- this ^^^^^^^^^^^ is dangerous
if (!isspace(*str++))
return 0;
- return 1;
+}
- static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data) { bool use_dbus = (bool)data;
@@ -192,15 +205,25 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data time_buf[time_len] = '\0'; }
- const char *not_reportable_reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE);
- const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON);
const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE);
if (is_null_or_mepty_string(reason)) /* if problem is NOT not reportable we use reason */
{
reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON);
if (is_null_or_mepty_string(reason)) /* if we don't have reason we use 'N/A' */
reason = "N/A";
}
/* the source of the problem:
- first we try to load component, as we use it on Fedora
*/ const char *source = problem_data_get_content_or_NULL(pd, FILENAME_COMPONENT);
- if (!source) /* if we don't have component, we fallback to executable */
if (is_null_or_mepty_string(source)) /* if we don't have component, we fallback to executable */
{ source = problem_data_get_content_or_NULL(pd, FILENAME_EXECUTABLE);
if (is_null_or_mepty_string(source)) /* even if we don't have executable, we use 'N/A' */
source = "N/A";
}
const char *msg = problem_data_get_content_or_NULL(pd, FILENAME_REPORTED_TO); GtkListStore *list_store = s_dumps_list_store;
@@ -215,7 +238,7 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data gtk_list_store_append(list_store, &iter); gtk_list_store_set(list_store, &iter, COLUMN_SOURCE, source,
COLUMN_REASON, not_reportable_reason ? : reason,
COLUMN_REASON, reason, //OPTION: time format COLUMN_LATEST_CRASH_STR, time_buf, COLUMN_LATEST_CRASH, t,
- the "N/A" should appear in all columns where we don't have text to show
Signed-off-by: Jakub Filak jfilak@redhat.com --- src/gui-gtk/main.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/gui-gtk/main.c b/src/gui-gtk/main.c index 12b14e9..1dab44b 100644 --- a/src/gui-gtk/main.c +++ b/src/gui-gtk/main.c @@ -180,18 +180,22 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data return; }
- char time_buf[sizeof("YYYY-MM-DD hh:mm:ss")]; - time_buf[0] = '\0'; const char *time_str = problem_data_get_content_or_NULL(pd, FILENAME_TIME); - time_t t = 0; - if (time_str && time_str[0]) + char *endptr; + errno = 0; + time_t t = strtol(time_str, &endptr, 10); + if (errno != 0 || endptr == time_str || *endptr != '\0') { - t = strtol(time_str, NULL, 10); /* atoi won't work past 2038! */ - struct tm *ptm = localtime(&t); - size_t time_len = strftime(time_buf, sizeof(time_buf)-1, "%Y-%m-%d %H:%M", ptm); - time_buf[time_len] = '\0'; + /* we don't use error_msg() because we don't want to bother user */ + log("'%s' is not valid problem because of invalid 'time' field ('%s')", problem_dir_path, time_str); + goto finis_add_directory; }
+ struct tm *ptm = localtime(&t); + char time_buf[sizeof("YYYY-MM-DD hh:mm:ss")]; + size_t time_len = strftime(time_buf, sizeof(time_buf)-1, "%Y-%m-%d %H:%M", ptm); + time_buf[time_len] = '\0'; + const char *not_reportable_reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE); const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON);
@@ -225,9 +229,11 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data -1);
free(subm_status); - problem_data_free(pd);
VERB1 log("added: %s", problem_dir_path); + +finis_add_directory: + problem_data_free(pd); }
static void query_dbus_and_add_to_dirlist(void)
Signed-off-by: Jakub Filak jfilak@redhat.com --- src/gui-gtk/main.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/src/gui-gtk/main.c b/src/gui-gtk/main.c index 1dab44b..e78eb1d 100644 --- a/src/gui-gtk/main.c +++ b/src/gui-gtk/main.c @@ -153,6 +153,12 @@ static void watch_this_dir(const char *dir_name) } }
+/* Returns non 0 if str is NULL or if str consists from white spaces */ +inline static int is_null_or_empty_string(const char *str) +{ + return !str || skip_whitespace(str)[0] == '\0'; +} + static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data) { bool use_dbus = (bool)data; @@ -196,15 +202,25 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data size_t time_len = strftime(time_buf, sizeof(time_buf)-1, "%Y-%m-%d %H:%M", ptm); time_buf[time_len] = '\0';
- const char *not_reportable_reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE); - const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON); + const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE); + if (is_null_or_empty_string(reason)) /* if problem is NOT not reportable we use reason */ + { + reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON); + if (is_null_or_empty_string(reason)) /* if we don't have reason we use 'N/A' */ + reason = "N/A"; + }
/* the source of the problem: * - first we try to load component, as we use it on Fedora */ const char *source = problem_data_get_content_or_NULL(pd, FILENAME_COMPONENT); - if (!source) /* if we don't have component, we fallback to executable */ + if (is_null_or_empty_string(source)) /* if we don't have component, we fallback to executable */ + { source = problem_data_get_content_or_NULL(pd, FILENAME_EXECUTABLE); + if (is_null_or_empty_string(source)) /* even if we don't have executable, we use 'N/A' */ + source = "N/A"; + } + const char *msg = problem_data_get_content_or_NULL(pd, FILENAME_REPORTED_TO);
GtkListStore *list_store = s_dumps_list_store; @@ -213,13 +229,19 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data { list_store = s_reported_dumps_list_store; subm_status = get_last_line(msg); + if (is_null_or_empty_string(subm_status)) + { /* the problem is reported but the submission status is not available */ + /* we have to use 'N/A' string instead of empty string */ + free(subm_status); + subm_status = xstrdup("N/A"); + } }
GtkTreeIter iter; gtk_list_store_append(list_store, &iter); gtk_list_store_set(list_store, &iter, COLUMN_SOURCE, source, - COLUMN_REASON, not_reportable_reason ? : reason, + COLUMN_REASON, reason, //OPTION: time format COLUMN_LATEST_CRASH_STR, time_buf, COLUMN_LATEST_CRASH, t,
On 07/31/2012 03:40 PM, Jakub Filak wrote:
Signed-off-by: Jakub Filak jfilak@redhat.com
src/gui-gtk/main.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/gui-gtk/main.c b/src/gui-gtk/main.c index 12b14e9..1dab44b 100644 --- a/src/gui-gtk/main.c +++ b/src/gui-gtk/main.c @@ -180,18 +180,22 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data return; }
- char time_buf[sizeof("YYYY-MM-DD hh:mm:ss")];
- time_buf[0] = '\0'; const char *time_str = problem_data_get_content_or_NULL(pd, FILENAME_TIME);
- time_t t = 0;
- if (time_str && time_str[0])
- char *endptr;
- errno = 0;
- time_t t = strtol(time_str, &endptr, 10);
- if (errno != 0 || endptr == time_str || *endptr != '\0') {
t = strtol(time_str, NULL, 10); /* atoi won't work past 2038! */
struct tm *ptm = localtime(&t);
size_t time_len = strftime(time_buf, sizeof(time_buf)-1, "%Y-%m-%d %H:%M", ptm);
time_buf[time_len] = '\0';
/* we don't use error_msg() because we don't want to bother user */
log("'%s' is not valid problem because of invalid 'time' field ('%s')", problem_dir_path, time_str);
goto finis_add_directory;
- typo "finis" :)
}
- struct tm *ptm = localtime(&t);
- char time_buf[sizeof("YYYY-MM-DD hh:mm:ss")];
- size_t time_len = strftime(time_buf, sizeof(time_buf)-1, "%Y-%m-%d %H:%M", ptm);
- time_buf[time_len] = '\0';
const char *not_reportable_reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE); const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON);
@@ -225,9 +229,11 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data -1);
free(subm_status);
problem_data_free(pd);
VERB1 log("added: %s", problem_dir_path);
+finis_add_directory:
problem_data_free(pd); }
static void query_dbus_and_add_to_dirlist(void)
- the check for the correct time should be in dd_opendir not in gui
Signed-off-by: Jakub Filak jfilak@redhat.com --- src/gui-gtk/main.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/src/gui-gtk/main.c b/src/gui-gtk/main.c index 12b14e9..2cc4bef 100644 --- a/src/gui-gtk/main.c +++ b/src/gui-gtk/main.c @@ -153,6 +153,12 @@ static void watch_this_dir(const char *dir_name) } }
+/* Returns non 0 if str is NULL or if str consists from white spaces */ +inline static int is_null_or_empty_string(const char *str) +{ + return !str || skip_whitespace(str)[0] == '\0'; +} + static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data) { bool use_dbus = (bool)data; @@ -192,15 +198,25 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data time_buf[time_len] = '\0'; }
- const char *not_reportable_reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE); - const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON); + const char *reason = problem_data_get_content_or_NULL(pd, FILENAME_NOT_REPORTABLE); + if (is_null_or_empty_string(reason)) /* if problem is NOT not reportable we use reason */ + { + reason = problem_data_get_content_or_NULL(pd, FILENAME_REASON); + if (is_null_or_empty_string(reason)) /* if we don't have reason we use 'N/A' */ + reason = "N/A"; + }
/* the source of the problem: * - first we try to load component, as we use it on Fedora */ const char *source = problem_data_get_content_or_NULL(pd, FILENAME_COMPONENT); - if (!source) /* if we don't have component, we fallback to executable */ + if (is_null_or_empty_string(source)) /* if we don't have component, we fallback to executable */ + { source = problem_data_get_content_or_NULL(pd, FILENAME_EXECUTABLE); + if (is_null_or_empty_string(source)) /* even if we don't have executable, we use 'N/A' */ + source = "N/A"; + } + const char *msg = problem_data_get_content_or_NULL(pd, FILENAME_REPORTED_TO);
GtkListStore *list_store = s_dumps_list_store; @@ -209,13 +225,19 @@ static void add_directory_to_dirlist(const char *problem_dir_path, gpointer data { list_store = s_reported_dumps_list_store; subm_status = get_last_line(msg); + if (is_null_or_empty_string(subm_status)) + { /* the problem is reported but the submission status is not available */ + /* we have to use 'N/A' string instead of empty string */ + free(subm_status); + subm_status = xstrdup("N/A"); + } }
GtkTreeIter iter; gtk_list_store_append(list_store, &iter); gtk_list_store_set(list_store, &iter, COLUMN_SOURCE, source, - COLUMN_REASON, not_reportable_reason ? : reason, + COLUMN_REASON, reason, //OPTION: time format COLUMN_LATEST_CRASH_STR, time_buf, COLUMN_LATEST_CRASH, t,
* the old implementation checks only presence of the time file * now we check validity of the time file
Signed-off-by: Jakub Filak jfilak@redhat.com --- src/lib/dump_dir.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 3 deletions(-)
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c index eef01f7..e945bad 100644 --- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -109,6 +109,57 @@ static bool exist_file_dir(const char *path) return false; }
+static bool not_valid_time_file(const char *filename) +{ + /* Open input file, and parse it. */ + int fd = open(filename, O_RDONLY); + if (fd < 0) + { + VERB1 log("Unable to open '%s': %s.\n", filename, strerror(errno)); + return true; + } + + off_t size = lseek(fd, 0, SEEK_END); + if (size == (off_t)-1) /* EOVERFLOW? */ + { + VERB1 log("Unable to seek in '%s': %s.\n", filename, strerror(errno)); + close(fd); + return true; + } + + lseek(fd, 0, SEEK_SET); /* No reason to fail. */ + + static const size_t FILE_SIZE_LIMIT = sizeof(time_t) * 3; /* ~ maximal number of digits */ + if (size > FILE_SIZE_LIMIT) + { + VERB1 log("Input file too big (%lld). Maximum size is %zd.\n", + (long long)size, FILE_SIZE_LIMIT); + close(fd); + return true; + } + + char *time_str = xmalloc(size + 1); + if (size != read(fd, time_str, size)) + { + VERB1 log("Unable to read from '%s'.\n", filename); + close(fd); + free(time_str); + return true; + } + + /* Just reading, so no need to check the returned value. */ + close(fd); + + time_str[size] = '\0'; + + /* range should OK because of file size condition above */ + const bool invalidity = !isdigit_str(time_str); + + free(time_str); + + return invalidity; +} + /* Return values: * -1: error (in this case, errno is 0 if error message is already logged) * 0: failed to lock (someone else has it locked) @@ -203,7 +254,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) if (sleep_usec == WAIT_FOR_OTHER_PROCESS_USLEEP) /* yes */ { strcpy(lock_buf + dirname_len, "/"FILENAME_TIME); - if (access(lock_buf, F_OK) != 0) + if (not_valid_time_file(lock_buf)) { /* time file doesn't exist. We managed to lock the directory * which was just created by somebody else, or is almost deleted @@ -212,7 +263,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) */ strcpy(lock_buf + dirname_len, "/.lock"); xunlink(lock_buf); - VERB1 log("Unlocked '%s' (no time file)", lock_buf); + VERB1 log("Unlocked '%s' (no or corrupted time file)", lock_buf); if (--count == 0) { errno = EISDIR; /* "this is an ordinary dir, not dump dir" */ @@ -303,7 +354,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags) && access(dir, R_OK) == 0 ) { char *time_file_name = concat_path_file(dir, FILENAME_TIME); - if (access(time_file_name, R_OK) != 0) + if (not_valid_time_file(time_file_name)) { dd_close(dd); dd = NULL;
On 07/31/2012 05:43 PM, Jakub Filak wrote:
- the old implementation checks only presence of the time file
- now we check validity of the time file
Signed-off-by: Jakub Filak jfilak@redhat.com
src/lib/dump_dir.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 3 deletions(-)
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c index eef01f7..e945bad 100644 --- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -109,6 +109,57 @@ static bool exist_file_dir(const char *path) return false; }
+static bool not_valid_time_file(const char *filename) +{
- /* Open input file, and parse it. */
- int fd = open(filename, O_RDONLY);
- if (fd < 0)
- {
VERB1 log("Unable to open '%s': %s.\n", filename, strerror(errno));
The "\n" is added by log functions themselves. I think you need this:
perror_msg("Can't open '%s'", filename);
return true;
- }
- off_t size = lseek(fd, 0, SEEK_END);
- if (size == (off_t)-1) /* EOVERFLOW? */
- {
VERB1 log("Unable to seek in '%s': %s.\n", filename, strerror(errno));
close(fd);
return true;
- }
- lseek(fd, 0, SEEK_SET); /* No reason to fail. */
- static const size_t FILE_SIZE_LIMIT = sizeof(time_t) * 3; /* ~ maximal number of digits */
- if (size > FILE_SIZE_LIMIT)
- {
VERB1 log("Input file too big (%lld). Maximum size is %zd.\n",
(long long)size, FILE_SIZE_LIMIT);
close(fd);
return true;
- }
You do not need to seek. Do read() of sizeof(time_t) * 3 + 2 bytes right away. If it reads exactly sizeof(time_t) * 3 + 2 bytes, then the file is too big.
- char *time_str = xmalloc(size + 1);
Use "char buf[sizeof(time_t) * 3 + 2]" instead: faster than xmalloc+free.
- if (size != read(fd, time_str, size))
- {
VERB1 log("Unable to read from '%s'.\n", filename);
close(fd);
free(time_str);
return true;
- }
- /* Just reading, so no need to check the returned value. */
- close(fd);
Can have just one close() instead of two:
ssize_t rdsz = read(fd, time_str, sizeof(buf)); close(fd); if (...) ...
- time_str[size] = '\0';
- /* range should OK because of file size condition above */
- const bool invalidity = !isdigit_str(time_str);
- free(time_str);
- return invalidity;
+}
* the old implementation checks only presence of the time file * now we check validity of the time file
Signed-off-by: Jakub Filak jfilak@redhat.com --- src/lib/dump_dir.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 3 deletions(-)
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c index eef01f7..303954c 100644 --- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -109,6 +109,58 @@ static bool exist_file_dir(const char *path) return false; }
+/* Returns true if the file is not readable or */ +/* if the file doesn't contain valid unixt time stamp */ +/* implementation is a compromise between efficiency and accuracy */ +/* the function gets false negative results for big numbers */ +static bool not_valid_time_file(const char *filename) +{ + /* Open input file, and parse it. */ + int fd = open(filename, O_RDONLY); + if (fd < 0) + { + VERB2 perror_msg("Can't open '%s'", filename); + return true; + } + + /* ~ maximal number of digits for positive time stamp string*/ + char time_buf[sizeof(time_t) * 3 + 1]; + const ssize_t rdsz = read(fd, time_buf, sizeof(time_buf)); + + /* Just reading, so no need to check the returned value. */ + close(fd); + + if (rdsz == -1) + { + VERB2 perror_msg("Can't read from '%s'", filename); + return true; + } + + /* approximate maximal number of digits in timestamp is sizeof(time_t)*3 */ + /* buffer has this size + 1 byte for trailing '\0' */ + /* if whole size of buffer was read then file is bigger */ + /* than string representing maximal time stamp */ + if (rdsz == sizeof(time_buf)) + { + VERB2 log("File '%s' is too long to be valid unix " + "time stamp (max size %zdB)", filename, sizeof(time_buf)); + return true; + } + + time_buf[rdsz] = '\0'; + + /* range should be OK because of file size condition above */ + /* hence check if the string consists only from digits */ + if (!isdigit_str(time_buf)) + { + VERB2 log("File '%s' doesn't contain valid unix " + "time stamp ('%s')", filename, time_buf); + return true; + } + + return false; +} + /* Return values: * -1: error (in this case, errno is 0 if error message is already logged) * 0: failed to lock (someone else has it locked) @@ -203,7 +255,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) if (sleep_usec == WAIT_FOR_OTHER_PROCESS_USLEEP) /* yes */ { strcpy(lock_buf + dirname_len, "/"FILENAME_TIME); - if (access(lock_buf, F_OK) != 0) + if (not_valid_time_file(lock_buf)) { /* time file doesn't exist. We managed to lock the directory * which was just created by somebody else, or is almost deleted @@ -212,7 +264,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) */ strcpy(lock_buf + dirname_len, "/.lock"); xunlink(lock_buf); - VERB1 log("Unlocked '%s' (no time file)", lock_buf); + VERB1 log("Unlocked '%s' (no or corrupted time file)", lock_buf); if (--count == 0) { errno = EISDIR; /* "this is an ordinary dir, not dump dir" */ @@ -303,7 +355,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags) && access(dir, R_OK) == 0 ) { char *time_file_name = concat_path_file(dir, FILENAME_TIME); - if (access(time_file_name, R_OK) != 0) + if (not_valid_time_file(time_file_name)) { dd_close(dd); dd = NULL;
On 08/01/2012 03:32 PM, Jakub Filak wrote:
- the old implementation checks only presence of the time file
- now we check validity of the time file
Signed-off-by: Jakub Filak jfilak@redhat.com
src/lib/dump_dir.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 3 deletions(-)
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c index eef01f7..303954c 100644 --- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -109,6 +109,58 @@ static bool exist_file_dir(const char *path) return false; }
+/* Returns true if the file is not readable or */ +/* if the file doesn't contain valid unixt time stamp */ +/* implementation is a compromise between efficiency and accuracy */ +/* the function gets false negative results for big numbers */ +static bool not_valid_time_file(const char *filename) +{
- /* Open input file, and parse it. */
- int fd = open(filename, O_RDONLY);
- if (fd < 0)
- {
VERB2 perror_msg("Can't open '%s'", filename);
return true;
- }
- /* ~ maximal number of digits for positive time stamp string*/
- char time_buf[sizeof(time_t) * 3 + 1];
- const ssize_t rdsz = read(fd, time_buf, sizeof(time_buf));
- /* Just reading, so no need to check the returned value. */
- close(fd);
- if (rdsz == -1)
Out of paranoia, I usually do (rdsz < 0) check instead.
- {
VERB2 perror_msg("Can't read from '%s'", filename);
return true;
- }
- /* approximate maximal number of digits in timestamp is sizeof(time_t)*3 */
- /* buffer has this size + 1 byte for trailing '\0' */
- /* if whole size of buffer was read then file is bigger */
- /* than string representing maximal time stamp */
- if (rdsz == sizeof(time_buf))
- {
VERB2 log("File '%s' is too long to be valid unix "
"time stamp (max size %zdB)", filename, sizeof(time_buf));
return true;
- }
- time_buf[rdsz] = '\0';
I would do this instead:
/* Our tools don't put trailing newline into time file, * but we allow such format too: */ if (rdsz > 0 && time_buf[rdsz - 1] == '\n') rdsz--; time_buf[rdsz] = '\0';
- /* range should be OK because of file size condition above */
What does this comment mean? "range"?
- /* hence check if the string consists only from digits */
- if (!isdigit_str(time_buf))
- {
VERB2 log("File '%s' doesn't contain valid unix "
"time stamp ('%s')", filename, time_buf);
return true;
- }
- return false;
+}
/* Return values:
- -1: error (in this case, errno is 0 if error message is already logged)
- 0: failed to lock (someone else has it locked)
@@ -203,7 +255,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) if (sleep_usec == WAIT_FOR_OTHER_PROCESS_USLEEP) /* yes */ { strcpy(lock_buf + dirname_len, "/"FILENAME_TIME);
if (access(lock_buf, F_OK) != 0)
if (not_valid_time_file(lock_buf)) { /* time file doesn't exist. We managed to lock the directory * which was just created by somebody else, or is almost deleted
@@ -212,7 +264,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags) */ strcpy(lock_buf + dirname_len, "/.lock"); xunlink(lock_buf);
VERB1 log("Unlocked '%s' (no time file)", lock_buf);
VERB1 log("Unlocked '%s' (no or corrupted time file)", lock_buf); if (--count == 0) { errno = EISDIR; /* "this is an ordinary dir, not dump dir" */
@@ -303,7 +355,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags) && access(dir, R_OK) == 0 ) { char *time_file_name = concat_path_file(dir, FILENAME_TIME);
if (access(time_file_name, R_OK) != 0)
if (not_valid_time_file(time_file_name)) { dd_close(dd); dd = NULL;
On Friday 03 of August 2012 16:40:07 Denys Vlasenko wrote:
On 08/01/2012 03:32 PM, Jakub Filak wrote:
- the old implementation checks only presence of the time file
- now we check validity of the time file
Signed-off-by: Jakub Filak jfilak@redhat.com
src/lib/dump_dir.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 3 deletions(-)
diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c index eef01f7..303954c 100644 --- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -109,6 +109,58 @@ static bool exist_file_dir(const char *path)
return false;
}
+/* Returns true if the file is not readable or */ +/* if the file doesn't contain valid unixt time stamp */ +/* implementation is a compromise between efficiency and accuracy */ +/* the function gets false negative results for big numbers */ +static bool not_valid_time_file(const char *filename) +{
- /* Open input file, and parse it. */
- int fd = open(filename, O_RDONLY);
- if (fd < 0)
- {
VERB2 perror_msg("Can't open '%s'", filename);
return true;
- }
- /* ~ maximal number of digits for positive time stamp string*/
- char time_buf[sizeof(time_t) * 3 + 1];
- const ssize_t rdsz = read(fd, time_buf, sizeof(time_buf));
- /* Just reading, so no need to check the returned value. */
- close(fd);
- if (rdsz == -1)
Out of paranoia, I usually do (rdsz < 0) check instead.
- {
VERB2 perror_msg("Can't read from '%s'", filename);
return true;
- }
- /* approximate maximal number of digits in timestamp is
sizeof(time_t)*3 */ + /* buffer has this size + 1 byte for trailing '\0' */
- /* if whole size of buffer was read then file is bigger */
- /* than string representing maximal time stamp */
- if (rdsz == sizeof(time_buf))
- {
VERB2 log("File '%s' is too long to be valid unix "
"time stamp (max size %zdB)", filename,
sizeof(time_buf)); + return true;
- }
- time_buf[rdsz] = '\0';
I would do this instead:
/* Our tools don't put trailing newline into time file, * but we allow such format too: */ if (rdsz > 0 && time_buf[rdsz - 1] == '\n') rdsz--; time_buf[rdsz] = '\0';
- /* range should be OK because of file size condition above */
What does this comment mean? "range"?
I tried to write a note about number range.
- /* hence check if the string consists only from digits */
- if (!isdigit_str(time_buf))
- {
VERB2 log("File '%s' doesn't contain valid unix "
"time stamp ('%s')", filename, time_buf);
return true;
- }
- return false;
+}
/* Return values:
- -1: error (in this case, errno is 0 if error message is already
logged)
- 0: failed to lock (someone else has it locked)
@@ -203,7 +255,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags)> if (sleep_usec == WAIT_FOR_OTHER_PROCESS_USLEEP) /* yes */ {
strcpy(lock_buf + dirname_len, "/"FILENAME_TIME);
if (access(lock_buf, F_OK) != 0)
if (not_valid_time_file(lock_buf)) { /* time file doesn't exist. We managed to lock the directory * which was just created by somebody else, or is almost deleted
@@ -212,7 +264,7 @@ static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags)> */
strcpy(lock_buf + dirname_len, "/.lock"); xunlink(lock_buf);
VERB1 log("Unlocked '%s' (no time file)", lock_buf);
VERB1 log("Unlocked '%s' (no or corrupted time file)",
lock_buf);> if (--count == 0) {
errno = EISDIR; /* "this is an ordinary dir, not dump dir" */
@@ -303,7 +355,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
&& access(dir, R_OK) == 0 ) { char *time_file_name = concat_path_file(dir, FILENAME_TIME);
if (access(time_file_name, R_OK) != 0)
if (not_valid_time_file(time_file_name)) { dd_close(dd); dd = NULL;
crash-catcher@lists.fedorahosted.org