Denys Vlasenko <dvlasenk(a)redhat.com> writes:
On Tue, 2011-08-09 at 15:24 +0200, Nikola Pajkovsky wrote:
> Denys Vlasenko <dvlasenk(a)redhat.com> writes:
>
> > On Tue, 2011-08-09 at 11:34 +0200, Nikola Pajkovsky wrote:
> >> Signed-off-by: Nikola Pajkovsky <npajkovs(a)redhat.com>
> >
> > Why is this needed?
>
> Because of configurable DumpLoction. When you specify location
> e.g. DumpLocaion = /selinux than the output from abrt-cli looks like
>
> $ abrt-cli list /selinux
> Directory: /selinux/policy_capabilities
>
> Directory: /selinux/class
>
> Directory: /selinux/initial_contexts
>
> Directory: /selinux/avc
>
> Directory: /selinux/booleans
>
> See the picture how gui looks here
>
https://bugzilla.redhat.com/show_bug.cgi?id=728166
Can't reproduce. I run "abrt-gui /selinux"
and I don't see anything out of ordinary.
Anyway, we already have a check for directory being not
a random directory. It is here:
static int dd_lock(struct dump_dir *dd, unsigned sleep_usec, int flags)
{
if (dd->locked)
error_msg_and_die("Locking bug on '%s'", dd->dd_dirname);
char pid_buf[sizeof(long)*3 + 2];
sprintf(pid_buf, "%lu", (long)getpid());
unsigned dirname_len = strlen(dd->dd_dirname);
char lock_buf[dirname_len + sizeof("/.lock")];
strcpy(lock_buf, dd->dd_dirname);
strcpy(lock_buf + dirname_len, "/.lock");
unsigned count = NO_TIME_FILE_COUNT;
retry:
while (1)
{
int r = get_and_set_lock(lock_buf, pid_buf);
if (r < 0)
return r; /* error */
if (r > 0)
break; /* locked successfully */
/* Other process has the lock, wait for it to go away */
usleep(sleep_usec);
}
/* Are we called by dd_opendir (as opposed to dd_create)? */
if (sleep_usec == WAIT_FOR_OTHER_PROCESS_USLEEP) /* yes */
{
strcpy(lock_buf + dirname_len, "/time");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
if (access(lock_buf, F_OK) != 0)
{
/* time file doesn't exist. We managed to lock the directory
* which was just created by somebody else, or is almost
deleted
* by delete_file_dir.
* Unlock and back off.
*/
strcpy(lock_buf + dirname_len, "/.lock");
xunlink(lock_buf);
VERB1 log("Unlocked '%s' (no time file)", lock_buf);
if (--count == 0)
{
errno = EISDIR; /* "this is an ordinary dir, not dump
dir" */
return -1;
}
usleep(NO_TIME_FILE_USLEEP);
goto retry;
}
}
We check that file named "time" exists.
Please also read comments on top of dump_dir.c.
If you can reproduce the problem on your machine,
can you determine why the above check doesn't work?
The problem is with function 'create_problem_data_from_dump_dir' which
never returns NULL.
problem_data_t *fill_crash_info(const char *dump_dir_name)
{
int sv_logmode = logmode;
logmode = 0; /* suppress EPERM/EACCES errors in opendir */
struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ DD_OPEN_READONLY);
logmode = sv_logmode;
if (!dd)
return NULL;
problem_data_t *problem_data = create_problem_data_from_dump_dir(dd);
^^^ it's allocated, but size of hash table is 0
dd_close(dd);
add_to_problem_data_ext(problem_data, CD_DUMPDIR, dump_dir_name,
CD_FLAG_TXT + CD_FLAG_ISNOTEDITABLE + CD_FLAG_LIST);
^^^ add dump dir name into empty hash table
return problem_data;
}
that leads to showing even directory what we should not handle
-----------------------------------------------
$ abrt-cli list /selinux
Directory: /selinux/policy_capabilities
Directory: /selinux/class
Directory: /selinux/initial_contexts
Directory: /selinux/avc
Directory: /selinux/booleans
-----------------------------------------------
I see 2 ways how to fix it
1)
problem_data_t *create_problem_data_from_dump_dir(struct dump_dir *dd)
{
problem_data_t *problem_data = new_problem_data();
load_problem_data_from_dump_dir(problem_data, dd, NULL);
if (g_hash_table_size(problem_data))
return problem_data;
else
free_table and return NULL;
}
2) fix load_problem_data_from_dump_dir to return NULL and rewrite
'create_problem_data_from_dump_dir' to
problem_data_t *create_problem_data_from_dump_dir(struct dump_dir *dd)
{
problem_data_t *problem_data = load_problem_data_from_dump_dir(dd, NULL);
return problem_data;
}
Your patch adds the check to copy_file_recursive().
This looks like a wrong place for such check:
this function is meant to be able to copy
any directory, not necessarily a problem dir.
Yes it is, I fixed, but anyway I dropped the whole patch, because it's
all wrong.
--
Save the whales. Feed the hungry. Free the mallocs