On Wed, 2011-08-10 at 12:58 +0200, Nikola Pajkovsky wrote:
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'm still confused. For example, cli-report.c:
int report(const char *dump_dir_name, int flags)
{
/* Load problem_data from (possibly updated by analyze) dump dir */
struct dump_dir *dd = dd_opendir(dump_dir_name, /*flags:*/ 0);
if (!dd)
return -1;
...
char *report_events_as_lines = list_possible_events(dd, NULL, "report");
problem_data_t *problem_data = create_problem_data_from_dump_dir(dd);
dd_close(dd);
The non-problem data dir should result in dd == NULL,
because dd_opendir() requires that "time" file exists.
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;
}
This is better than this:
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;
}
but it should be unnecessary anyway...
We need first to understand why current safeguard doesn't work.
--
vda