Re: Bug#369305: zsh: failed to write history file /home/sven/.zsh-history: no such file or directory

Re: Bug#369305: zsh: failed to write history file /home/sven/.zsh-history: no such file or directory

[Clint Adams]

On Wed, Mar 05, 2008 at 01:35:59PM +0100, Sven Joachim wrote:
> I'm noticing the odd message in the subject when exiting from "sudo zsh".
> However, the file ~/.zsh_history is actually updated anyway.  And despite
> the claims in bug #347444, its ownership is not changed, fortunately.

That error message is indeed misleading.

Re: Bug#369305: zsh: failed to write history file /home/sven/.zsh-history: no such file or directory

[Peter Stephenson]

Clint Adams wrote:
> On Wed, Mar 05, 2008 at 01:35:59PM +0100, Sven Joachim wrote:
> > I'm noticing the odd message in the subject when exiting from "sudo zsh".
> > However, the file ~/.zsh_history is actually updated anyway.  And despite
> > the claims in bug #347444, its ownership is not changed, fortunately.
> 
> That error message is indeed misleading.

You haven't said how to reproduce this (all I've seen is above), but
there are now lots tests in savehistfile() in hist.c for failure to
write and one of them is being overzealous.  Possibly ret is being set
to -1 without errno being set by the corresponding call, but it sounds
like there's more to it than that.  This should be easy enough to track
down if you can get it to happen.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: Bug#369305: zsh: failed to write history file /home/sven/.zsh-history: no such file or directory

[Clint Adams]

On Wed, Mar 05, 2008 at 05:12:23PM +0000, Peter Stephenson wrote:
> You haven't said how to reproduce this (all I've seen is above), but
> there are now lots tests in savehistfile() in hist.c for failure to
> write and one of them is being overzealous.  Possibly ret is being set
> to -1 without errno being set by the corresponding call, but it sounds
> like there's more to it than that.  This should be easy enough to track
> down if you can get it to happen.

With extendedhistory and sharehistory on (I haven't tried anything
else), do a 'sudo -s' or 'sudo zsh', then exit the shell.

As Sven notes, any history added before exiting will make it into the
history file despite the error message.

Re: Bug#369305: zsh: failed to write history file /home/sven/.zsh-history: no such file or directory

[Wayne Davison]

On Wed, Mar 05, 2008 at 03:29:36PM -0500, Clint Adams wrote:
> As Sven notes, any history added before exiting will make it into the
> history file despite the error message.

The history being added is the appending going on due to SHARE_HISTORY.
The error is referring to the inability to rewrite the history file on
exit, and it is actually failing (because the write-new, then rename
steps would cause the file to change ownership).  If incremental history
updating was not happening, the failed rewrite would indeed not save
anything new.  The "no such file or directory" error string is
presumably a left-over errno that Peter noticed.

Aside:  I like to make sure to never mix user history and root history,
so I define a different HISTFILE for root (e.g. .zhist_$USER).

..wayne..

Re: Bug#369305: zsh: failed to write history file /home/sven/.zsh-history: no such file or directory

[Peter Stephenson]

On Wed, 5 Mar 2008 14:41:58 -0800
Wayne Davison <wayned@users.sourceforge.net> wrote:
> On Wed, Mar 05, 2008 at 03:29:36PM -0500, Clint Adams wrote:
> > As Sven notes, any history added before exiting will make it into the
> > history file despite the error message.
> 
> The history being added is the appending going on due to SHARE_HISTORY.
> The error is referring to the inability to rewrite the history file on
> exit, and it is actually failing (because the write-new, then rename
> steps would cause the file to change ownership).  If incremental history
> updating was not happening, the failed rewrite would indeed not save
> anything new.  The "no such file or directory" error string is
> presumably a left-over errno that Peter noticed.

Yes, I've found where... at this point we've already handled the error in
the manner you say by simply not trying to write the history, so there's no
system error we haven't dealt with and we should reset errno.

We could fix both problems by not printing an error if there's no "errno",
as below.  That's OK in the specific circumstances, but probably not right
in all cases.  Could we, for example, pass down a flag when savehistfile()
is called from itself to do the rewriting and suppress the error message,
or make it more anodyne, in that case?

If we decide a warning message is still appropriate, given that we've
detected exactly what's going on it should probably be more specific than
just a vague "failed to write history".

I won't commit this until we've resolved the TODO, but you might want to
check it does remove the error message.

Index: Src/hist.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/hist.c,v
retrieving revision 1.70
diff -u -r1.70 hist.c
--- Src/hist.c	31 Dec 2007 23:14:17 -0000	1.70
+++ Src/hist.c	6 Mar 2008 10:16:08 -0000
@@ -2207,7 +2207,13 @@
 	    struct stat sb;
 	    int old_exists = stat(unmeta(fn), &sb) == 0;
 
+	    errno = 0;
 	    if (old_exists && sb.st_uid != geteuid()) {
+		/*
+		 * TODO: do we want an error message about changed ownership
+		 * here?  Do we want a lesser error message, or none,
+		 * if this call to savehistfile() was just a rewrite?
+		 */
 		free(tmpfile);
 		tmpfile = NULL; /* Avoid an error about HISTFILE.new */
 		out = NULL;
@@ -2302,7 +2308,7 @@
     } else
 	ret = -1;
 
-    if (ret < 0 && err) {
+    if (ret < 0 && err && errno) {
 	if (tmpfile) {
 	    zerr("failed to write history file %s.new: %e", fn, errno);
 	    free(tmpfile);

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: Bug#369305: zsh: failed to write history file /home/sven/.zsh-history: no such file or directory

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 973 bytes --]

On Thu, Mar 06, 2008 at 10:19:37AM +0000, Peter Stephenson wrote:
> If we decide a warning message is still appropriate, given that we've
> detected exactly what's going on it should probably be more specific
> than just a vague "failed to write history".

Yes, we should definitely be giving the user better information.

I've fixed several things:

1. Tell the user when we're refusing to write out the history file so
that the ownership won't change.  Try to differentiate if we're just
skipping the rewrite or if no new history data was written.

2. Allow root to write out the file even if it doesn't own it, as long
as zsh knows how to chown a file handle.

3. Avoid the generic failure message that confuses the issue (by setting
"err = 0").

4. (bonus) Output the right error if we can't open the file for writing,
not an error about the file-handle being invalid.  (This was due to the
use of fdopen(open(...), ...), which loses the errno from open().)

..wayne..

[-- Attachment #2: hist-fixes.patch --]
[-- Type: text/plain, Size: 1856 bytes --]

--- Src/hist.c	31 Dec 2007 23:14:17 -0000	1.70
+++ Src/hist.c	6 Mar 2008 16:43:05 -0000
@@ -2192,13 +2192,13 @@ savehistfile(char *fn, int err, int writ
     }
     errno = 0;
     if (writeflags & HFILE_APPEND) {
+	int fd = open(unmeta(fn), O_CREAT | O_WRONLY | O_APPEND | O_NOCTTY, 0600);
 	tmpfile = NULL;
-	out = fdopen(open(unmeta(fn),
-			O_CREAT | O_WRONLY | O_APPEND | O_NOCTTY, 0600), "a");
+	out = fd >= 0 ? fdopen(fd, "a") : NULL;
     } else if (!isset(HISTSAVEBYCOPY)) {
+	int fd = open(unmeta(fn), O_CREAT | O_WRONLY | O_TRUNC | O_NOCTTY, 0600);
 	tmpfile = NULL;
-	out = fdopen(open(unmeta(fn),
-			 O_CREAT | O_WRONLY | O_TRUNC | O_NOCTTY, 0600), "w");
+	out = fd >= 0 ? fdopen(fd, "w") : NULL;
     } else {
 	tmpfile = bicat(unmeta(fn), ".new");
 	if (unlink(tmpfile) < 0 && errno != ENOENT)
@@ -2206,13 +2206,27 @@ savehistfile(char *fn, int err, int writ
 	else {
 	    struct stat sb;
 	    int old_exists = stat(unmeta(fn), &sb) == 0;
+	    uid_t euid = geteuid();
 
-	    if (old_exists && sb.st_uid != geteuid()) {
+	    if (old_exists
+#if defined HAVE_FCHMOD && defined HAVE_FCHOWN
+	     && euid
+#endif
+	     && sb.st_uid != euid) {
 		free(tmpfile);
-		tmpfile = NULL; /* Avoid an error about HISTFILE.new */
+		if (err) {
+		    if (isset(APPENDHISTORY) || isset(INCAPPENDHISTORY)
+		     || isset(SHAREHISTORY))
+			zerr("writing %s would change its ownership -- skipped size trimming", fn);
+		    else
+			zerr("writing %s would change its ownership -- history not saved", fn);
+		    err = 0; /* Don't report a generic error below. */
+		}
 		out = NULL;
-	    } else
-		out = fdopen(open(tmpfile, O_CREAT | O_WRONLY | O_EXCL, 0600), "w");
+	    } else {
+		int fd = open(tmpfile, O_CREAT | O_WRONLY | O_EXCL, 0600);
+		out = fd >= 0 ? fdopen(fd, "w") : NULL;
+	    }
 
 #ifdef HAVE_FCHMOD
 	    if (old_exists && out) {

Re: Bug#369305: zsh: failed to write history file /home/sven/.zsh-history: no such file or directory

[Wayne Davison]

On Thu, Mar 06, 2008 at 09:02:00AM -0800, Wayne Davison wrote:
> +			zerr("writing %s would change its ownership -- skipped size trimming", fn);
> +		    else
> +			zerr("writing %s would change its ownership -- history not saved", fn);

I had changed those errors, but failed to create a new patch.  They now read:

zerr("rewriting %s would change its ownership -- skipped", fn);

and

zerr("rewriting %s would change its ownership -- history not saved", fn);

..wayne..