zsh doesn't load HISTFILE from readonly directory

zsh doesn't load HISTFILE from readonly directory

[Jörg Sommer]

Hi,

if the directory, containing the history file, is not writeable, zsh
doesn't load the data and I've not history. I've no problem that zsh
can't save the file, but I would like to have the history from the file.

% ls -ld /tmp/aaa
dr-xr-xr-x 2 joerg users 80 13. Jun 15:00 /tmp/aaa

% ls -l /tmp/aaa/.z*
-r-------- 1 joerg users 93 13. Jun 15:00 /tmp/aaa/.zhistfile
-rw-r--r-- 1 joerg users 26 13. Jun 15:00 /tmp/aaa/.zshrc

<1,5029>(ibook):~% cat /tmp/aaa/.zhistfile
: 1276430398:1;ls
: 1276430403:0;cd /tmp
: 1276430407:0;ls
: 1276430427:0;exec $ZSH_NAME -$-

% cat /tmp/aaa/.zshrc
HISTFILE=$HOME/.zhistfile

With this version, you get the history.

% HOME=/tmp/aaa zsh
ibook% fc -l
    1  ls
    2  cd /tmp
    3  ls
    4  exec $ZSH_NAME -$-
ibook% echo $ZSH_VERSION 
4.3.10

But with this version, the history is empty.

% HOME=/tmp/aaa zsh-beta
ibook% fc -l
    1  fc -l
ibook% echo $ZSH_VERSION 
4.3.10-dev-1-cvs0603

Regards, Jörg
-- 
Gienger's Law (http://www.bruhaha.de/laws.html):
Die Wichtigkeit eines Newspostings im Usenet ist reziprok zur Anzahl der
enthaltenenen, kumulierten Ausrufungszeichen.

Re: zsh doesn't load HISTFILE from readonly directory

[Peter Stephenson]

On Sun, 13 Jun 2010 13:17:37 +0000 (UTC)
Jörg Sommer <joerg@alea.gnuu.de> wrote:
> if the directory, containing the history file, is not writeable, zsh
> doesn't load the data and I've not history.

Yes, it's returning after not being able to lock the file.  This is
unhealthy because (1) there's no error, which might be arguable for normal
interactive history but is certainly wrong for an explicit "fc -R" (2)
there's no reason why it shouldn't just read the history.

Not immediately sure how wide the scope of the fix should be.  Get locking
to check if the file is not writeable for some reason (which is a different
test from checking if it can be locked)?  Do we need to remember that we
didn't lock it to avoid errors later?  Or should we simply issue a warning
when and continue any time we can't lock the file?  That's simple but it's
not very safe if the file can be written and shared history is in use.

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


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: zsh doesn't load HISTFILE from readonly directory

[Peter Stephenson]

On Mon, 14 Jun 2010 15:05:21 +0100
Peter Stephenson <Peter.Stephenson@csr.com> wrote:

> On Sun, 13 Jun 2010 13:17:37 +0000 (UTC)
> Jörg Sommer <joerg@alea.gnuu.de> wrote:
> > if the directory, containing the history file, is not writeable, zsh
> > doesn't load the data and I've not history.
> 
> Yes, it's returning after not being able to lock the file.  This is
> unhealthy because (1) there's no error, which might be arguable for
> normal interactive history but is certainly wrong for an explicit "fc
> -R" (2) there's no reason why it shouldn't just read the history.
> 
> Not immediately sure how wide the scope of the fix should be.  Get
> locking to check if the file is not writeable for some reason (which
> is a different test from checking if it can be locked)?

That's no good.  In a case like this (where the problem is directory
permissions) we might well still be able to write to the file.  However,
as zsh wouldn't be able to lock it, it would refuse to write to it.
(There's a subtlety that fcntl() file locking might still work but if
you have inconsistent file locking options you're already stuffed.)

> Or should
> we simply issue a warning when and continue any time we can't lock
> the file?

So, as long as we narrow this to make a check for why we couldn't create
the lock (EACCES in this case) maybe this is the right way to go after all?

We still need to decide when we should print an error.  I would be tempted
to do it every time.

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


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: zsh doesn't load HISTFILE from readonly directory

[Peter Stephenson]

On Mon, 14 Jun 2010 15:16:26 +0100
Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> On Mon, 14 Jun 2010 15:05:21 +0100
> Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> > On Sun, 13 Jun 2010 13:17:37 +0000 (UTC)
> > Jörg Sommer <joerg@alea.gnuu.de> wrote:
> > > if the directory, containing the history file, is not writeable,
> > > zsh doesn't load the data and I've not history.
> > 
> > Yes, it's returning after not being able to lock the file.  This is
> > unhealthy because (1) there's no error, which might be arguable for
> > normal interactive history but is certainly wrong for an explicit
> > "fc -R" (2) there's no reason why it shouldn't just read the
> > history.
> > 
> > Not immediately sure how wide the scope of the fix should be.  Get
> > locking to check if the file is not writeable for some reason (which
> > is a different test from checking if it can be locked)?
> 
> That's no good.  In a case like this (where the problem is directory
> permissions) we might well still be able to write to the file.
> However, as zsh wouldn't be able to lock it, it would refuse to write
> to it. (There's a subtlety that fcntl() file locking might still work
> but if you have inconsistent file locking options you're already
> stuffed.)
> 
> > Or should
> > we simply issue a warning when and continue any time we can't lock
> > the file?
> 
> So, as long as we narrow this to make a check for why we couldn't
> create the lock (EACCES in this case) maybe this is the right way to
> go after all?
> 
> We still need to decide when we should print an error.  I would be
> tempted to do it every time.

This prints an error when fc fails, but not when interactive history
fails.  It probably wants careful looking over:  note the change of meaning
in return values from file locking.

Index: Src/hist.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/hist.c,v
retrieving revision 1.98
diff -p -u -r1.98 hist.c
--- Src/hist.c	22 Mar 2010 23:20:11 -0000	1.98
+++ Src/hist.c	17 Jun 2010 16:05:23 -0000
@@ -1194,7 +1194,7 @@ hend(Eprog prog)
     callhookfunc("zshaddhistory", hookargs, 1, &hookret);
     /* For history sharing, lock history file once for both read and write */
     hf = getsparam("HISTFILE");
-    if (isset(SHAREHISTORY) && lockhistfile(hf, 0)) {
+    if (isset(SHAREHISTORY) && !lockhistfile(hf, 0)) {
 	readhistfile(hf, 0, HFILE_USE_OPTIONS | HFILE_FAST);
 	curline.histnum = curhist+1;
     }
@@ -2231,20 +2231,25 @@ readhistfile(char *fn, int err, int read
     short *wordlist;
     struct stat sb;
     int nwordpos, nwordlist, bufsiz;
-    int searching, newflags, l;
+    int searching, newflags, l, ret;
 
     if (!fn && !(fn = getsparam("HISTFILE")))
 	return;
     if (readflags & HFILE_FAST) {
 	if (stat(unmeta(fn), &sb) < 0
 	 || (lasthist.fsiz == sb.st_size && lasthist.mtim == sb.st_mtime)
-	 || !lockhistfile(fn, 0))
+	 || lockhistfile(fn, 0))
 	    return;
 	lasthist.fsiz = sb.st_size;
 	lasthist.mtim = sb.st_mtime;
+    } else if ((ret = lockhistfile(fn, 1))) {
+	if (ret == 2) {
+	    zwarn("locking failed for %s: %e: reading anyway", fn, errno);
+	} else {
+	    zerr("locking failed for %s: %e", fn, errno);
+	    return;
+	}
     }
-    else if (!lockhistfile(fn, 1))
-	return;
     if ((in = fopen(unmeta(fn), "r"))) {
 	nwordlist = 64;
 	wordlist = (short *)zalloc(nwordlist*sizeof(short));
@@ -2377,6 +2382,11 @@ readhistfile(char *fn, int err, int read
 #ifdef HAVE_FCNTL_H
 static int flock_fd = -1;
 
+/*
+ * Lock file using fcntl().  Return 0 on success, 1 on failure of
+ * locking mechanism, 2 on permanent failure (e.g. permission).
+ */
+
 static int
 flockhistfile(char *fn, int keep_trying)
 {
@@ -2384,7 +2394,7 @@ flockhistfile(char *fn, int keep_trying)
     int ctr = keep_trying ? 9 : 0;
 
     if ((flock_fd = open(unmeta(fn), O_RDWR | O_NOCTTY)) < 0)
-	return errno == ENOENT; /* "successfully" locked missing file */
+	return errno == ENOENT ? 0 : 2; /* "successfully" locked missing file */
 
     lck.l_type = F_WRLCK;
     lck.l_whence = SEEK_SET;
@@ -2395,12 +2405,12 @@ flockhistfile(char *fn, int keep_trying)
 	if (--ctr < 0) {
 	    close(flock_fd);
 	    flock_fd = -1;
-	    return 0;
+	    return 1;
 	}
 	sleep(1);
     }
 
-    return 1;
+    return 0;
 }
 #endif
 
@@ -2424,14 +2434,16 @@ savehistfile(char *fn, int err, int writ
 	    lasthist.next_write_ev = he->histnum + 1;
 	    he = down_histent(he);
 	}
-	if (!he || !lockhistfile(fn, 0))
+	if (!he || lockhistfile(fn, 0))
 	    return;
 	if (histfile_linect > savehistsiz + savehistsiz / 5)
 	    writeflags &= ~HFILE_FAST;
     }
     else {
-	if (!lockhistfile(fn, 1))
+	if (lockhistfile(fn, 1)) {
+	    zerr("locking failed for %s: %e", fn, errno);
 	    return;
+	}
 	he = hist_ring->down;
     }
     if (writeflags & HFILE_USE_OPTIONS) {
@@ -2597,18 +2609,27 @@ savehistfile(char *fn, int err, int writ
 
 static int lockhistct;
 
+/*
+ * Lock history file.  Return 0 on success, 1 on failure to lock this
+ * time, 2 on permanent failure (e.g. permission).
+ */
+
 /**/
 int
 lockhistfile(char *fn, int keep_trying)
 {
     int ct = lockhistct;
+    int ret = 0;
 
     if (!fn && !(fn = getsparam("HISTFILE")))
-	return 0;
+	return 1;
 
 #ifdef HAVE_FCNTL_H
-    if (isset(HISTFCNTLLOCK) && flock_fd < 0 && !flockhistfile(fn, keep_trying))
-	return 0;
+    if (isset(HISTFCNTLLOCK) && flock_fd < 0) {
+	ret = flockhistfile(fn, keep_trying);
+	if (ret)
+	    return ret;
+    }
 #endif
 
     if (!lockhistct++) {
@@ -2632,8 +2653,13 @@ lockhistfile(char *fn, int keep_trying)
 	lnk = bicat(pidbuf, getsparam("HOST"));
 	/* We'll abuse fd as our success flag. */
 	while ((fd = symlink(lnk, lockfile)) < 0) {
-	    if (errno != EEXIST || !keep_trying)
+	    if (errno != EEXIST) {
+		ret = 2;
+		break;
+	    } else if (!keep_trying) {
+		ret = 1;
 		break;
+	    }
 	    if (lstat(lockfile, &sb) < 0) {
 		if (errno == ENOENT)
 		    continue;
@@ -2656,15 +2682,16 @@ lockhistfile(char *fn, int keep_trying)
 	    } else
 		close(fd);
 	    while (link(tmpfile, lockfile) < 0) {
-		if (errno != EEXIST)
-		    zerr("failed to create hard link as lock file %s: %e",
-			 lockfile, errno);
-		else if (!keep_trying)
-		    ;
-		else if (lstat(lockfile, &sb) < 0) {
+		if (errno != EEXIST) {
+		    ret = 2;
+		    break;
+		} else if (!keep_trying) {
+		    ret = 1;
+		    break;
+		} else if (lstat(lockfile, &sb) < 0) {
 		    if (errno == ENOENT)
 			continue;
-		    zerr("failed to stat lock file %s: %e", lockfile, errno);
+		    ret = 2;
 		} else {
 		    if (time(NULL) - sb.st_mtime < 10)
 			sleep(1);
@@ -2681,11 +2708,17 @@ lockhistfile(char *fn, int keep_trying)
 # endif /* not HAVE_SYMLINK */
 #else /* not HAVE_LINK */
 	while ((fd = open(lockfile, O_WRONLY|O_CREAT|O_EXCL, 0644)) < 0) {
-	    if (errno != EEXIST || !keep_trying)
+	    if (errno != EEXIST) {
+		ret = 2;
 		break;
+	    } else if (!keep_trying) {
+		ret = 1;
+		break;
+	    }
 	    if (lstat(lockfile, &sb) < 0) {
 		if (errno == ENOENT)
 		    continue;
+		ret = 2;
 		break;
 	    }
 	    if (time(NULL) - sb.st_mtime < 10)
@@ -2714,9 +2747,10 @@ lockhistfile(char *fn, int keep_trying)
 	    flock_fd = -1;
 	}
 #endif
-	return 0;
+	DPUTS(ret == 0, "BUG: return value non-zero on locking error");
+	return ret;
     }
-    return 1;
+    return 0;
 }
 
 /* Unlock the history file if this corresponds to the last nested lock

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


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom

Re: zsh doesn't load HISTFILE from readonly directory

[Peter Stephenson]

On Thu, 17 Jun 2010 17:07:30 +0100
Peter Stephenson <Peter.Stephenson@csr.com> wrote:
> > > Or should
> > > we simply issue a warning when and continue any time we can't lock
> > > the file?
> > 
> > So, as long as we narrow this to make a check for why we couldn't
> > create the lock (EACCES in this case) maybe this is the right way to
> > go after all?
> > 
> > We still need to decide when we should print an error.  I would be
> > tempted to do it every time.
> 
> This prints an error when fc fails, but not when interactive history
> fails.  It probably wants careful looking over:  note the change of
> meaning in return values from file locking.

I've committed this, so watch for oddities to do with history reading and
writing.

(I also committed a change to _zstyle by mistake which is harmless but
currently useless.  It should become more useful after I post some new
shell functions.)

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


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom