Re: [PATCH] mc crashes when temporary directory cannot be created
Hi Pavel, On Sat, 2006-12-30 at 20:51 +0200, Pavel Tsekov wrote: There is an even simpler cure. In mc_tmpdir() when executing the fallback code pass an absolute path to mc_mkstemps(). This will prevent the loop. However I am not yet conviced that this is the best solution. I am still investigating. I am attaching a patch which passes an absolute path to mc_mkstemps() when invoked from mc_tmpdir(). What do you think about this fix ? I may add a comment why it is necessary to call mc_mkstemps() with an absolute path. By the way I think we should add a check whether the environment variable TMPDIR contains an absolute path. Anyway, I'll leave this for another patch. Thanks for the patch. Do you plan to convert the relative paths to absolute ones when detected? Jindrich ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
Hello, On Tue, 2 Jan 2007, Jindrich Novy wrote: On Sat, 2006-12-30 at 20:51 +0200, Pavel Tsekov wrote: There is an even simpler cure. In mc_tmpdir() when executing the fallback code pass an absolute path to mc_mkstemps(). This will prevent the loop. However I am not yet conviced that this is the best solution. I am still investigating. I am attaching a patch which passes an absolute path to mc_mkstemps() when invoked from mc_tmpdir(). What do you think about this fix ? I may add a comment why it is necessary to call mc_mkstemps() with an absolute path. By the way I think we should add a check whether the environment variable TMPDIR contains an absolute path. Anyway, I'll leave this for another patch. Thanks for the patch. Do you plan to convert the relative paths to absolute ones when detected? I think TMPDIR should be ignored in this case. What's your opinion ? ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
Hi Pavel, On Tue, 2007-01-02 at 16:28 +0200, Pavel Tsekov wrote: On Tue, 2 Jan 2007, Jindrich Novy wrote: On Sat, 2006-12-30 at 20:51 +0200, Pavel Tsekov wrote: There is an even simpler cure. In mc_tmpdir() when executing the fallback code pass an absolute path to mc_mkstemps(). This will prevent the loop. However I am not yet conviced that this is the best solution. I am still investigating. I am attaching a patch which passes an absolute path to mc_mkstemps() when invoked from mc_tmpdir(). What do you think about this fix ? I may add a comment why it is necessary to call mc_mkstemps() with an absolute path. By the way I think we should add a check whether the environment variable TMPDIR contains an absolute path. Anyway, I'll leave this for another patch. Thanks for the patch. Do you plan to convert the relative paths to absolute ones when detected? I think TMPDIR should be ignored in this case. What's your opinion ? So some hardcoded value such as /tmp will then be assumed in case of relative path in TMPDIR? Jindrich ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
Hello,
On Tue, 2 Jan 2007, Jindrich Novy wrote:
On Tue, 2007-01-02 at 16:28 +0200, Pavel Tsekov wrote:
On Tue, 2 Jan 2007, Jindrich Novy wrote:
On Sat, 2006-12-30 at 20:51 +0200, Pavel Tsekov wrote:
There is an even simpler cure. In mc_tmpdir() when executing
the fallback code pass an absolute path to mc_mkstemps().
This will prevent the loop. However I am not yet conviced
that this is the best solution. I am still investigating.
I am attaching a patch which passes an absolute path to mc_mkstemps()
when invoked from mc_tmpdir(). What do you think about this fix ?
I may add a comment why it is necessary to call mc_mkstemps() with
an absolute path. By the way I think we should add a check whether
the environment variable TMPDIR contains an absolute path. Anyway, I'll
leave this for another patch.
Thanks for the patch. Do you plan to convert the relative paths to
absolute ones when detected?
I think TMPDIR should be ignored in this case. What's your opinion ?
So some hardcoded value such as /tmp will then be assumed in case of
relative path in TMPDIR?
Yep. It already is:
sys_tmp = getenv (TMPDIR);
if (!sys_tmp) {
sys_tmp = TMPDIR_DEFAULT;
}
___
Mc-devel mailing list
http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
On Tue, 2 Jan 2007, Jindrich Novy wrote: On Tue, 2007-01-02 at 16:28 +0200, Pavel Tsekov wrote: On Tue, 2 Jan 2007, Jindrich Novy wrote: On Sat, 2006-12-30 at 20:51 +0200, Pavel Tsekov wrote: There is an even simpler cure. In mc_tmpdir() when executing the fallback code pass an absolute path to mc_mkstemps(). This will prevent the loop. However I am not yet conviced that this is the best solution. I am still investigating. I am attaching a patch which passes an absolute path to mc_mkstemps() when invoked from mc_tmpdir(). What do you think about this fix ? I may add a comment why it is necessary to call mc_mkstemps() with an absolute path. By the way I think we should add a check whether the environment variable TMPDIR contains an absolute path. Anyway, I'll leave this for another patch. Thanks for the patch. Do you plan to convert the relative paths to absolute ones when detected? I think TMPDIR should be ignored in this case. What's your opinion ? So some hardcoded value such as /tmp will then be assumed in case of relative path in TMPDIR? Btw. does it make sense to use relative TMPDIR ?! ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
Hi Pavel,
On Tue, 2007-01-02 at 16:48 +0200, Pavel Tsekov wrote:
On Tue, 2 Jan 2007, Jindrich Novy wrote:
On Tue, 2007-01-02 at 16:28 +0200, Pavel Tsekov wrote:
On Tue, 2 Jan 2007, Jindrich Novy wrote:
On Sat, 2006-12-30 at 20:51 +0200, Pavel Tsekov wrote:
There is an even simpler cure. In mc_tmpdir() when executing
the fallback code pass an absolute path to mc_mkstemps().
This will prevent the loop. However I am not yet conviced
that this is the best solution. I am still investigating.
I am attaching a patch which passes an absolute path to mc_mkstemps()
when invoked from mc_tmpdir(). What do you think about this fix ?
I may add a comment why it is necessary to call mc_mkstemps() with
an absolute path. By the way I think we should add a check whether
the environment variable TMPDIR contains an absolute path. Anyway, I'll
leave this for another patch.
Thanks for the patch. Do you plan to convert the relative paths to
absolute ones when detected?
I think TMPDIR should be ignored in this case. What's your opinion ?
So some hardcoded value such as /tmp will then be assumed in case of
relative path in TMPDIR?
Yep. It already is:
sys_tmp = getenv (TMPDIR);
if (!sys_tmp) {
sys_tmp = TMPDIR_DEFAULT;
}
It sounds good for me then ;-)
Jindrich
___
Mc-devel mailing list
http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
On Tue, 2007-01-02 at 16:53, Jindrich Novy wrote:
I am attaching a patch which passes an absolute path to mc_mkstemps()
when invoked from mc_tmpdir(). What do you think about this fix ?
I may add a comment why it is necessary to call mc_mkstemps() with
an absolute path. By the way I think we should add a check whether
the environment variable TMPDIR contains an absolute path. Anyway, I'll
leave this for another patch.
Thanks for the patch. Do you plan to convert the relative paths to
absolute ones when detected?
I think TMPDIR should be ignored in this case. What's your opinion ?
So some hardcoded value such as /tmp will then be assumed in case of
relative path in TMPDIR?
Yep. It already is:
sys_tmp = getenv (TMPDIR);
if (!sys_tmp) {
sys_tmp = TMPDIR_DEFAULT;
}
It sounds good for me then ;-)
The patch is in CVS now. I'll add a check for TMPDIR being an absolute
path later today.
___
Mc-devel mailing list
http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
Hello Jindrich,
On Thu, 2006-11-30 at 20:04, Pavel Tsekov wrote:
On Wed, 29 Nov 2006, Leonard den Ottolander wrote:
Hello Jindrich,
On Tue, 2006-11-28 at 13:21 +0100, Jindrich Novy wrote:
IMO only removal of the fallback will prevent
the infinite loop in any case as it shouldn't call mc_mkstemps() at all.
That cure seems worse than the disease. Isn't the real problem the fact
that mc_mkstemps() blindly concats tmpdir to the prefix instead of
testing if mc_tmpdir() succeeded? Why not abort mc_mkstemps() if
mc_tmpdir() returns /dev/null/?
What about the attached (untested) patch?
By the way, could you please add the -p option to your diffs?
There is an even simpler cure. In mc_tmpdir() when executing
the fallback code pass an absolute path to mc_mkstemps().
This will prevent the loop. However I am not yet conviced
that this is the best solution. I am still investigating.
I am attaching a patch which passes an absolute path to mc_mkstemps()
when invoked from mc_tmpdir(). What do you think about this fix ?
I may add a comment why it is necessary to call mc_mkstemps() with
an absolute path. By the way I think we should add a check whether
the environment variable TMPDIR contains an absolute path. Anyway, I'll
leave this for another patch.
Index: src/utilunix.c
===
RCS file: /sources/mc/mc/src/utilunix.c,v
retrieving revision 1.88
diff -u -p -r1.88 utilunix.c
--- src/utilunix.c 27 Jul 2005 15:03:25 - 1.88
+++ src/utilunix.c 30 Dec 2006 18:44:22 -
@@ -280,19 +280,18 @@ mc_tmpdir (void)
}
}
-if (!error) {
- tmpdir = buffer;
-} else {
+if (error != NULL) {
int test_fd;
- char *test_fn;
+ char *test_fn, *fallback_prefix;
int fallback_ok = 0;
if (*error)
fprintf (stderr, error, buffer);
/* Test if sys_tmp is suitable for temporary files */
- tmpdir = sys_tmp;
- test_fd = mc_mkstemps (test_fn, mctest, NULL);
+ fallback_prefix = g_strdup_printf (%s/mctest, sys_tmp);
+ test_fd = mc_mkstemps (test_fn, fallback_prefix, NULL);
+ g_free (fallback_prefix);
if (test_fd != -1) {
close (test_fd);
test_fd = open (test_fn, O_RDONLY);
@@ -306,16 +305,19 @@ mc_tmpdir (void)
if (fallback_ok) {
fprintf (stderr, _(Temporary files will be created in %s\n),
sys_tmp);
+ g_snprintf (buffer, sizeof (buffer), %s, sys_tmp);
error = NULL;
} else {
fprintf (stderr, _(Temporary files will not be created\n));
- tmpdir = /dev/null/;
+ g_snprintf (buffer, sizeof (buffer), %s, /dev/null/);
}
fprintf (stderr, %s\n, _(Press any key to continue...));
getc (stdin);
}
+tmpdir = buffer;
+
if (!error)
mc_setenv (MC_TMPDIR, tmpdir, 1);
___
Mc-devel mailing list
http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
Hi Leonard, On Wed, 2006-11-29 at 20:59 +0100, Leonard den Ottolander wrote: Hello Jindrich, On Tue, 2006-11-28 at 13:21 +0100, Jindrich Novy wrote: IMO only removal of the fallback will prevent the infinite loop in any case as it shouldn't call mc_mkstemps() at all. That cure seems worse than the disease. Isn't the real problem the fact that mc_mkstemps() blindly concats tmpdir to the prefix instead of testing if mc_tmpdir() succeeded? Why not abort mc_mkstemps() if mc_tmpdir() returns /dev/null/? I'm not quite sure I got the point. We may want to prevent infinite loops caused by the subsequent function calls, such design is dangerous by design. The fallback will never work without adding hacks to mc_mktemps()/mc_tmpdir() to check for special arguments what doesn't look too nice to me. Considering that insufficient space in TMPDIR is very rare, I would vote for removing the dangerous fallback completely what is done in the previous patch. mc works fine after that even without the fallback. What about the attached (untested) patch? $ TMPDIR=/dev/null mc Cannot create temporary directory /dev/null/mc-jnovy: Not a directory (20) Cannot create temporary directory /dev/null/mc-jnovy: Not a directory (20) snip Cannot create temporary directory /dev/null/mc-jnovy: Not a directory (20) Cannot create temporary directory /dev/null/mc-jnovy: Not a directory (20) Segmentation fault By the way, could you please add the -p option to your diffs? Sure. Jindrich ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
On Wed, 29 Nov 2006, Leonard den Ottolander wrote: Hello Jindrich, On Tue, 2006-11-28 at 13:21 +0100, Jindrich Novy wrote: IMO only removal of the fallback will prevent the infinite loop in any case as it shouldn't call mc_mkstemps() at all. That cure seems worse than the disease. Isn't the real problem the fact that mc_mkstemps() blindly concats tmpdir to the prefix instead of testing if mc_tmpdir() succeeded? Why not abort mc_mkstemps() if mc_tmpdir() returns /dev/null/? What about the attached (untested) patch? By the way, could you please add the -p option to your diffs? There is an even simpler cure. In mc_tmpdir() when executing the fallback code pass an absolute path to mc_mkstemps(). This will prevent the loop. However I am not yet conviced that this is the best solution. I am still investigating. ___ Mc-devel mailing list http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
Hello Jindrich,
On Tue, 2006-11-28 at 13:21 +0100, Jindrich Novy wrote:
IMO only removal of the fallback will prevent
the infinite loop in any case as it shouldn't call mc_mkstemps() at all.
That cure seems worse than the disease. Isn't the real problem the fact
that mc_mkstemps() blindly concats tmpdir to the prefix instead of
testing if mc_tmpdir() succeeded? Why not abort mc_mkstemps() if
mc_tmpdir() returns /dev/null/?
What about the attached (untested) patch?
By the way, could you please add the -p option to your diffs?
Bye,
Leonard.
--
mount -t life -o ro /dev/dna /genetic/research
--- util.c.000 2005-11-10 21:50:22.0 +0100
+++ util.c 2006-11-29 20:56:21.0 +0100
@@ -1301,14 +1301,18 @@ mc_mkstemps (char **pname, const char *p
= abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789;
static unsigned long value;
struct timeval tv;
+char *tmpdir;
char *tmpbase;
char *tmpname;
char *XX;
int count;
if (strchr (prefix, PATH_SEP) == NULL) {
+ tmpdir = mc_tmpdir ();
+ if (strcmp (tmpdir, /dev/null/) == 0)
+ return -1;
/* Add prefix first to find the position of XX */
- tmpbase = concat_dir_and_file (mc_tmpdir (), prefix);
+ tmpbase = concat_dir_and_file (tmpdir, prefix);
} else {
tmpbase = g_strdup (prefix);
}
___
Mc-devel mailing list
http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
Hi Pavel,
On Mon, 2006-11-27 at 17:37 +0200, Pavel Tsekov wrote:
Hello Jindrich,
On Mon, 27 Nov 2006, Jindrich Novy wrote:
there is a breakage in util.c and utilunix.c related to temporary files
creation. The problem is that if a directory for temporary files cannot
be created mc ends up in infinite loop caused by:
tmpbase = concat_dir_and_file (mc_tmpdir (), prefix);
in mc_mkstemps() which then calls mc_tmpdir() back infinitely and ends
up in a stack underflow.
The attached patch fixes it as it disables the creation of the temporary
files when the temp. directory couldn't be created.
Ok. But... what happens if any of the following
error conditions occur ?
if (lstat (buffer, st) == 0) {
/* Sanity check for existing directory */
if (!S_ISDIR (st.st_mode))
error = _(%s is not a directory\n);
else if (st.st_uid != getuid ())
error = _(Directory %s is not owned by you\n);
else if (((st.st_mode 0777) != 0700)
(chmod (buffer, 0700) != 0))
error = _(Cannot set correct permissions for directory
%s\n);
} else {
Wouldn't it cause the same loop as when mkdir() fails ?
Good point, I missed that. IMO only removal of the fallback will prevent
the infinite loop in any case as it shouldn't call mc_mkstemps() at all.
I'm sending patch for it.
Thoughts?
Jindrich
--- mc-2006-11-14-16/src/utilunix.c.tmpcrash 2005-07-27 17:03:25.0 +0200
+++ mc-2006-11-14-16/src/utilunix.c 2006-11-28 13:12:36.0 +0100
@@ -283,34 +283,11 @@
if (!error) {
tmpdir = buffer;
} else {
- int test_fd;
- char *test_fn;
- int fallback_ok = 0;
-
if (*error)
fprintf (stderr, error, buffer);
- /* Test if sys_tmp is suitable for temporary files */
- tmpdir = sys_tmp;
- test_fd = mc_mkstemps (test_fn, mctest, NULL);
- if (test_fd != -1) {
- close (test_fd);
- test_fd = open (test_fn, O_RDONLY);
- if (test_fd != -1) {
- close (test_fd);
- unlink (test_fn);
- fallback_ok = 1;
- }
- }
-
- if (fallback_ok) {
- fprintf (stderr, _(Temporary files will be created in %s\n),
- sys_tmp);
- error = NULL;
- } else {
- fprintf (stderr, _(Temporary files will not be created\n));
- tmpdir = /dev/null/;
- }
+ fprintf (stderr, _(Temporary files will not be created\n));
+ tmpdir = /dev/null/;
fprintf (stderr, %s\n, _(Press any key to continue...));
getc (stdin);
___
Mc-devel mailing list
http://mail.gnome.org/mailman/listinfo/mc-devel
[PATCH] mc crashes when temporary directory cannot be created
Hi all,
there is a breakage in util.c and utilunix.c related to temporary files
creation. The problem is that if a directory for temporary files cannot
be created mc ends up in infinite loop caused by:
tmpbase = concat_dir_and_file (mc_tmpdir (), prefix);
in mc_mkstemps() which then calls mc_tmpdir() back infinitely and ends
up in a stack underflow.
The attached patch fixes it as it disables the creation of the temporary
files when the temp. directory couldn't be created.
Cheers,
Jindrich
--- mc-2006-11-14-16/src/utilunix.c.tmpcrash 2005-07-27 17:03:25.0 +0200
+++ mc-2006-11-14-16/src/utilunix.c 2006-11-27 10:32:54.0 +0100
@@ -274,9 +274,12 @@
/* Need to create directory */
if (mkdir (buffer, S_IRWXU) != 0) {
fprintf (stderr,
- _(Cannot create temporary directory %s: %s\n),
- buffer, unix_error_string (errno));
- error = ;
+ _(Cannot create temporary directory %s: %s\n%s%s\n),
+ buffer, unix_error_string (errno),
+ _(Temporary files will not be created\n),
+ _(Press any key to continue...));
+ getc (stdin);
+ return /dev/null/;
}
}
___
Mc-devel mailing list
http://mail.gnome.org/mailman/listinfo/mc-devel
Re: [PATCH] mc crashes when temporary directory cannot be created
Hello Jindrich,
On Mon, 27 Nov 2006, Jindrich Novy wrote:
there is a breakage in util.c and utilunix.c related to temporary files
creation. The problem is that if a directory for temporary files cannot
be created mc ends up in infinite loop caused by:
tmpbase = concat_dir_and_file (mc_tmpdir (), prefix);
in mc_mkstemps() which then calls mc_tmpdir() back infinitely and ends
up in a stack underflow.
The attached patch fixes it as it disables the creation of the temporary
files when the temp. directory couldn't be created.
Ok. But... what happens if any of the following
error conditions occur ?
if (lstat (buffer, st) == 0) {
/* Sanity check for existing directory */
if (!S_ISDIR (st.st_mode))
error = _(%s is not a directory\n);
else if (st.st_uid != getuid ())
error = _(Directory %s is not owned by you\n);
else if (((st.st_mode 0777) != 0700)
(chmod (buffer, 0700) != 0))
error = _(Cannot set correct permissions for directory
%s\n);
} else {
Wouldn't it cause the same loop as when mkdir() fails ?
___
Mc-devel mailing list
http://mail.gnome.org/mailman/listinfo/mc-devel
