Jiri found a bug in previous patch, so here goes v.2
Before this patch, dd_opendir(".") was creating
lock file "..lock", which is obviously wrong.
This patch makes dd_opendir use realpath(dir).
This makes dd_opendir(".") and similar cases correctly
determine what's its parent directory and therefore
use correct lock file name.
dd_create can't use realpath, since it will always return NULL
on not-yet existing directory. But this is not a problem, since
dd_create(".") isn't a sane operation. So we continue to use
old code. I only added a sanity check to refuse names ending
in . and .. - just in case.
Run-tested.
Please review.
--
vda
diff -x '*.po' -d -urpN abrt.3/lib/utils/dump_dir.c abrt.4/lib/utils/dump_dir.c
--- abrt.3/lib/utils/dump_dir.c 2010-10-14 11:05:43.883114773 +0200
+++ abrt.4/lib/utils/dump_dir.c 2010-10-14 16:13:44.963483812 +0200
@@ -28,10 +28,6 @@
// Perhaps dd_opendir should do some sanity checking like
// "if there is no "uid" file in the directory, it's not a crash
dump",
// and fail.
-//
-// Locking is broken wrt "funny" directory names.
-// dd_opendir(dd, ".") will create "..lock" istead of proper
"../DIRNAME.lock"
-// Similarly for dd_opendir(dd, "DIRNAME/."), dd_opendir(dd, "..")
etc.
static char *load_text_file(const char *path);
@@ -45,14 +41,6 @@ static bool isdigit_str(const char *str)
return true;
}
-static char* rm_trailing_slashes(const char *dir)
-{
- unsigned len = strlen(dir);
- while (len != 0 && dir[len-1] == '/')
- len--;
- return xstrndup(dir, len);
-}
-
static bool exist_file_dir(const char *path)
{
struct stat buf;
@@ -171,12 +159,27 @@ void dd_close(struct dump_dir *dd)
free(dd);
}
+static char* rm_trailing_slashes(const char *dir)
+{
+ unsigned len = strlen(dir);
+ while (len != 0 && dir[len-1] == '/')
+ len--;
+ return xstrndup(dir, len);
+}
+
int dd_opendir(struct dump_dir *dd, const char *dir, int flags)
{
if (dd->locked)
error_msg_and_die("dump_dir is already opened"); /* bug */
- dd->dd_dir = rm_trailing_slashes(dir);
+ /* Used to use rm_trailing_slashes(dir) here, but with dir = "."
+ * or "..", or if the last component is a symlink,
+ * then lock file is created in the wrong place.
+ * IOW: this breaks locking.
+ */
+ dd->dd_dir = realpath(dir, NULL);
+ if (!dd->dd_dir)
+ goto err;
dd_lock(dd);
@@ -184,6 +187,7 @@ int dd_opendir(struct dump_dir *dd, cons
if (stat(dd->dd_dir, &stat_buf) != 0
|| !S_ISDIR(stat_buf.st_mode)
) {
+ err:
if (!(flags & DD_FAIL_QUIETLY))
error_msg("'%s' does not exist", dd->dd_dir);
if (flags & DD_CLOSE_ON_OPEN_ERR)
@@ -222,8 +226,26 @@ int dd_create(struct dump_dir *dd, const
if (dd->locked)
error_msg_and_die("dump_dir is already opened"); /* bug */
+ /* Unlike dd_opendir, can't use realpath: the directory doesn't exist yet,
+ * realpath will always return NULL. We don't really have to:
+ * dd_opendir(".") makes sense, dd_create(".") does not.
+ */
dd->dd_dir = rm_trailing_slashes(dir);
+ char *last_component = strrchr(dd->dd_dir, '/');
+ if (last_component)
+ last_component++;
+ else
+ last_component = dd->dd_dir;
+ if (dot_or_dotdot(last_component))
+ {
+ /* dd_create("."), dd_create(".."),
dd_create("dir/."),
+ * dd_create("dir/..") and similar are madness, refuse them.
+ */
+ error_msg("Bad dir name '%s'", dd->dd_dir);
+ return 0;
+ }
+
dd_lock(dd);
/* Was creating it with mode 0700 and user as the owner, but this allows