[RHEL6 LIBREPORT PATCH] Fix bugs discoverent by Coverity. rhbz#905051
Jiri Moskovcak
jmoskovc at redhat.com
Wed Jun 12 11:54:09 UTC 2013
Pushed, please update the bugzilla with the comment from the patch, so
QA engineers know what was fixed.
Thanks,
Jirka
On 06/12/2013 01:11 PM, Denys Vlasenko wrote:
> This is a partial backport of 93c6e6f9298928dcb140797d9fe8ff9038790423.
>
> Most bugs are either outright bogus (rhbz_array_size is never <0)
> or won't happen in practice (fstat won't fail on a file we just created
> ourselves). Nevertheless, one bug is real:
>
> if (flags & LIBREPORT_NOWAIT)...
>
> is WRONG, because LIBREPORT_NOWAIT constant is in fact 0!
> We must check for that bit this way:
>
> if (!(flags & LIBREPORT_WAIT))...
>
> Signed-off-by: Denys Vlasenko <dvlasenk at redhat.com>
> ---
> src/cli/cli-report.c | 9 ++++-----
> src/gui-wizard-gtk/wizard.c | 18 ++++++++++--------
> src/lib/dump_dir.c | 14 ++++++++++++--
> src/lib/report.c | 2 +-
> src/plugins/reporter-bugzilla.c | 4 ++--
> src/plugins/rhbz.c | 12 ++++++------
> src/plugins/rhbz.h | 2 +-
> 7 files changed, 36 insertions(+), 25 deletions(-)
>
> diff --git a/src/cli/cli-report.c b/src/cli/cli-report.c
> index 1e13a38..0e36150 100644
> --- a/src/cli/cli-report.c
> +++ b/src/cli/cli-report.c
> @@ -333,11 +333,10 @@ static int run_report_editor(problem_data_t *problem_data)
> return 2;
> }
>
> - fseek(fp, 0, SEEK_END);
> - unsigned long size = ftell(fp);
> - fseek(fp, 0, SEEK_SET);
> -
> - char *text = (char*)xmalloc(size + 1);
> + off_t size = fstat_st_size_or_die(fileno(fp));
> + if (size > INT_MAX/4)
> + size = INT_MAX/4; /* paranoia */
> + char *text = xmalloc(size + 1);
> if (fread(text, 1, size, fp) != size)
> {
> error_msg("Can't read '%s'", filename);
> diff --git a/src/gui-wizard-gtk/wizard.c b/src/gui-wizard-gtk/wizard.c
> index 57365bd..4820546 100644
> --- a/src/gui-wizard-gtk/wizard.c
> +++ b/src/gui-wizard-gtk/wizard.c
> @@ -1092,14 +1092,16 @@ static void append_item_to_ls_details(gpointer name, gpointer value, gpointer da
> {
> struct stat statbuf;
> statbuf.st_size = 0;
> - stat(item->content, &statbuf);
> - stats->filesize += statbuf.st_size;
> - char *msg = xasprintf(_("(binary file, %llu bytes)"), (long long)statbuf.st_size);
> - gtk_list_store_set(g_ls_details, &iter,
> - DETAIL_COLUMN_NAME, (char *)name,
> - DETAIL_COLUMN_VALUE, msg,
> - -1);
> - free(msg);
> + if (stat(item->content, &statbuf) == 0)
> + {
> + stats->filesize += statbuf.st_size;
> + char *msg = xasprintf(_("(binary file, %llu bytes)"), (long long)statbuf.st_size);
> + gtk_list_store_set(g_ls_details, &iter,
> + DETAIL_COLUMN_NAME, (char *)name,
> + DETAIL_COLUMN_VALUE, msg,
> + -1);
> + free(msg);
> + }
> }
>
> int cur_value;
> diff --git a/src/lib/dump_dir.c b/src/lib/dump_dir.c
> index f2e7376..e1af555 100644
> --- a/src/lib/dump_dir.c
> +++ b/src/lib/dump_dir.c
> @@ -287,7 +287,8 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
> dir = dd->dd_dirname = rm_trailing_slashes(dir);
>
> struct stat stat_buf;
> - stat(dir, &stat_buf);
> + if (stat(dir, &stat_buf) != 0)
> + goto cant_access;
> /* & 0666 should remove the executable bit */
> dd->mode = (stat_buf.st_mode & 0666);
>
> @@ -331,7 +332,10 @@ struct dump_dir *dd_opendir(const char *dir, int flags)
> else
> {
> if (!(flags & DD_FAIL_QUIETLY_EACCES))
> + {
> + cant_access:
> perror_msg("Can't access '%s'", dir);
> + }
> }
> dd_close(dd);
> return NULL;
> @@ -549,7 +553,13 @@ void dd_sanitize_mode_and_owner(struct dump_dir *dd)
> if (lstat(full_path, &statbuf) == 0 && S_ISREG(statbuf.st_mode))
> {
> if ((statbuf.st_mode & 0777) != dd->mode)
> - chmod(full_path, dd->mode);
> + {
> + if (chmod(full_path, dd->mode) != 0)
> + {
> + perror_msg("Can't change '%s' mode to 0%o", full_path,
> + (unsigned)dd->mode);
> + }
> + }
> if (statbuf.st_uid != dd->dd_uid || statbuf.st_gid != dd->dd_gid)
> {
> if (lchown(full_path, dd->dd_uid, dd->dd_gid) != 0)
> diff --git a/src/lib/report.c b/src/lib/report.c
> index 9e1d601..7b7deab 100644
> --- a/src/lib/report.c
> +++ b/src/lib/report.c
> @@ -175,7 +175,7 @@ int report_problem_in_memory(problem_data_t *pd, int flags)
> dd_close(dd);
> VERB2 log("Temp problem dir: '%s'", dir_name);
>
> - if (flags & LIBREPORT_NOWAIT)
> + if (!(flags & LIBREPORT_WAIT))
> flags |= LIBREPORT_DEL_DIR;
> result = report_problem_in_dir(dir_name, flags);
>
> diff --git a/src/plugins/reporter-bugzilla.c b/src/plugins/reporter-bugzilla.c
> index 7bde348..9374505 100644
> --- a/src/plugins/reporter-bugzilla.c
> +++ b/src/plugins/reporter-bugzilla.c
> @@ -165,7 +165,7 @@ int main(int argc, char **argv)
> xmlrpc_value *all_bugs = rhbz_get_member("bugs", result);
> xmlrpc_DECREF(result);
>
> - int all_bugs_size = rhbz_array_size(all_bugs);
> + unsigned all_bugs_size = rhbz_array_size(all_bugs);
> if (all_bugs_size > 0)
> {
> int bug_id = rhbz_bug_id(all_bugs);
> @@ -302,7 +302,7 @@ int main(int argc, char **argv)
> if (!all_bugs)
> error_msg_and_die(_("Missing mandatory member 'bugs'"));
>
> - int all_bugs_size = rhbz_array_size(all_bugs);
> + unsigned all_bugs_size = rhbz_array_size(all_bugs);
> // When someone clones bug it has same duphash, so we can find more than 1.
> // Need to be checked if component is same.
> VERB3 log("Bugzilla has %i reports with same duphash '%s'",
> diff --git a/src/plugins/rhbz.c b/src/plugins/rhbz.c
> index 359c15a..300c5c8 100644
> --- a/src/plugins/rhbz.c
> +++ b/src/plugins/rhbz.c
> @@ -154,11 +154,11 @@ static GList *parse_comments(xmlrpc_value *result_xml)
> if (!comments_memb)
> return NULL;
>
> - int comments_memb_size = rhbz_array_size(comments_memb);
> + unsigned comments_memb_size = rhbz_array_size(comments_memb);
>
> xmlrpc_env env;
> xmlrpc_env_init(&env);
> - for (int i = 0; i < comments_memb_size; ++i)
> + for (unsigned i = 0; i < comments_memb_size; ++i)
> {
> xmlrpc_value* item = NULL;
> xmlrpc_array_read_item(&env, comments_memb, i, &item);
> @@ -312,14 +312,14 @@ xmlrpc_value *rhbz_get_member(const char *member, xmlrpc_value *xml)
> * value. So it is usually not worth checking *envP.
> * die or return size of array
> */
> -int rhbz_array_size(xmlrpc_value *xml)
> +unsigned rhbz_array_size(xmlrpc_value *xml)
> {
> func_entry();
>
> xmlrpc_env env;
> xmlrpc_env_init(&env);
>
> - int size = xmlrpc_array_size(&env, xml);
> + unsigned size = xmlrpc_array_size(&env, xml);
> if (env.fault_occurred)
> abrt_xmlrpc_die(&env);
>
> @@ -419,12 +419,12 @@ GList *rhbz_bug_cc(xmlrpc_value* result_xml)
> if (!cc_member)
> return NULL;
>
> - int array_size = rhbz_array_size(cc_member);
> + unsigned array_size = rhbz_array_size(cc_member);
>
> VERB3 log("count members on cc %i", array_size);
> GList *cc_list = NULL;
>
> - for (int i = 0; i < array_size; ++i)
> + for (unsigned i = 0; i < array_size; ++i)
> {
> xmlrpc_value* item = NULL;
> xmlrpc_array_read_item(&env, cc_member, i, &item);
> diff --git a/src/plugins/rhbz.h b/src/plugins/rhbz.h
> index 10c37bc..585b23e 100644
> --- a/src/plugins/rhbz.h
> +++ b/src/plugins/rhbz.h
> @@ -93,7 +93,7 @@ xmlrpc_value *rhbz_search_duphash(struct abrt_xmlrpc *ax, const char *component,
>
> xmlrpc_value *rhbz_get_member(const char *member, xmlrpc_value *xml);
>
> -int rhbz_array_size(xmlrpc_value *xml);
> +unsigned rhbz_array_size(xmlrpc_value *xml);
>
> int rhbz_bug_id(xmlrpc_value *xml);
>
>
More information about the Crash-catcher
mailing list