PATCH: check deleted .zwc files

PATCH: check deleted .zwc files

[Clint Adams]

This seems to work; functions can be autoloaded from
deleted-but-still-open digest files.  Is any further
logic needed to prevent anything?

Index: zshconfig.ac
===================================================================
RCS file: /cvsroot/zsh/zsh/zshconfig.ac,v
retrieving revision 1.3
diff -u -r1.3 zshconfig.ac
--- zshconfig.ac	2001/06/06 16:55:23	1.3
+++ zshconfig.ac	2001/06/08 18:10:51
@@ -912,7 +912,8 @@
 
 AC_CHECK_FUNCS(strftime difftime gettimeofday \
 	       select poll \
-	       readlink lstat lchown faccessx fchdir ftruncate \
+	       readlink faccessx fchdir ftruncate \
+	       fstat lstat lchown \
 	       fseeko ftello \
 	       mkfifo _mktemp \
 	       waitpid wait3 \
Index: Src/parse.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/parse.c,v
retrieving revision 1.20
diff -u -r1.20 parse.c
--- Src/parse.c	2001/05/15 08:38:59	1.20
+++ Src/parse.c	2001/06/08 18:10:52
@@ -2827,6 +2827,7 @@
     d->addr = addr;
     d->len = len;
     d->count = 0;
+    d->filename = ztrdup(dump);
 }
 
 #endif
@@ -2854,7 +2855,7 @@
     dig = dyncat(path, FD_EXT);
     wc = dyncat(file, FD_EXT);
 
-    rd = stat(dig, &std);
+    rd = zwcstat(dig, &std, dumps);
     rc = stat(wc, &stc);
     rn = stat(file, &stn);
 
@@ -2934,7 +2935,7 @@
     struct stat lsbuf;
 
     if (!sbuf) {
-	if (stat(file, &lsbuf))
+	if (zwcstat(file, &lsbuf, dumps))
 	    return NULL;
 	sbuf = &lsbuf;
     }
@@ -3077,6 +3078,7 @@
 		dumps = p->next;
 	    munmap((void *) f->addr, f->len);
 	    zclose(f->fd);
+	    zsfree(f->filename);
 	    zfree(f, sizeof(*f));
 	}
     }
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.30
diff -u -r1.30 utils.c
--- Src/utils.c	2001/05/29 15:09:06	1.30
+++ Src/utils.c	2001/06/08 18:10:53
@@ -3829,3 +3829,20 @@
        return 0;
 }
 #endif
+
+/**/
+mod_export int
+zwcstat(char *filename, struct stat *buf, FuncDump dumps)
+{
+    FuncDump f;
+    
+    if (stat(filename, buf)) {
+#ifdef HAVE_FSTAT
+	for (f = dumps; f; f = f->next) {
+	    if (!fstat(f->fd, buf)) return 0;
+	}
+#endif
+	return 1;
+    } else return 0;
+    
+}
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.29
diff -u -r1.29 zsh.h
--- Src/zsh.h	2001/05/15 16:32:28	1.29
+++ Src/zsh.h	2001/06/08 18:10:53
@@ -493,6 +493,7 @@
     Wordcode addr;		/* mapped region */
     int len;			/* length */
     int count;			/* reference count */
+    char *filename;
 };
 
 struct eprog {

Re: PATCH: check deleted .zwc files

[Bart Schaefer]

On Jun 8,  2:17pm, Clint Adams wrote:
> Subject: PATCH: check deleted .zwc files
> 
> This seems to work; functions can be autoloaded from
> deleted-but-still-open digest files.  Is any further
> logic needed to prevent anything?

What happens if the file has been deleted and re-created?  zwcstat()
will return the stat info for the newly-created file, but what will
autoloading do with the now-obsolete (?) file handle?

Re: PATCH: check deleted .zwc files

[Clint Adams]

> What happens if the file has been deleted and re-created?  zwcstat()
> will return the stat info for the newly-created file, but what will
> autoloading do with the now-obsolete (?) file handle?

Oops.  It will do nothing with it, just as it did nothing with it before.
It'll check device/inode against the existing file rather than the
replaced one.

Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.31
diff -u -r1.31 utils.c
--- Src/utils.c	2001/06/08 18:34:53	1.31
+++ Src/utils.c	2001/06/08 19:03:05
@@ -3839,7 +3839,9 @@
     if (stat(filename, buf)) {
 #ifdef HAVE_FSTAT
 	for (f = dumps; f; f = f->next) {
-	    if (!fstat(f->fd, buf)) return 0;
+	    if (!strncmp(filename, f->filename, strlen(f->filename)) &&
+		!fstat(f->fd, buf))
+		return 0;
 	}
 #endif
 	return 1;

Re: PATCH: check deleted .zwc files

[Bart Schaefer]

On Jun 8,  3:04pm, Clint Adams wrote:
> Subject: Re: PATCH: check deleted .zwc files
> 
> > What happens if the file has been deleted and re-created?  zwcstat()
> > will return the stat info for the newly-created file, but what will
> > autoloading do with the now-obsolete (?) file handle?
> 
> Oops.  It will do nothing with it, just as it did nothing with it before.

Maybe this is a flaw in the whole .zwc loading scheme, then.

What I'm getting at is this scenario:

I have a .zwc file for several functions.  I update one of those functions.
I then remove and rebuild the .zwc file.  Now, stat() of the .zwc shows
that it is newer than the function file, but any running zsh still has a
mapped descriptor on the removed file.  With this code:

>      if (stat(filename, buf)) {
>  #ifdef HAVE_FSTAT
>  	for (f = dumps; f; f = f->next) {
> +	    if (!strncmp(filename, f->filename, strlen(f->filename)) &&
> +		!fstat(f->fd, buf))
> +		return 0;
>  	}
>  #endif
>  	return 1;

zwcstat() will cause zsh to decide that it can keep using the mapped copy,
even though the function file is actually newer than the mapped copy.

The only safe thing to do is to try the fstat() first, then stat().  To be
really complete, if fstat() and stat() disagree zsh should throw out the
mapped file and reload it, but that could get pretty tricky.

Re: PATCH: check deleted .zwc files

[Clint Adams]

> I have a .zwc file for several functions.  I update one of those functions.
> I then remove and rebuild the .zwc file.  Now, stat() of the .zwc shows
> that it is newer than the function file, but any running zsh still has a
> mapped descriptor on the removed file.  With this code:

> >      if (stat(filename, buf)) {
[...]

> zwcstat() will cause zsh to decide that it can keep using the mapped copy,
> even though the function file is actually newer than the mapped copy.

Maybe I'm confused.  If you've generated a new .zwc file, that stat()
will return 0, and the mapped copy won't even be considered.
 
> The only safe thing to do is to try the fstat() first, then stat().  To be
> really complete, if fstat() and stat() disagree zsh should throw out the
> mapped file and reload it, but that could get pretty tricky.

Too tricky for me.

RE: PATCH: check deleted .zwc files

[Andrej Borsenkow]

>
> Maybe I'm confused.  If you've generated a new .zwc file, that stat()
> will return 0, and the mapped copy won't even be considered.
>

You mean, it will be loaded (mapped) again? Then it is potential (albeit
slow) memory leak.

-andrej

Cygwin failure RE: PATCH: check deleted .zwc files

[Andrej Borsenkow]

gcc -c -I.  -DHAVE_CONFIG_H -Wall -Wno-implicit -Wmissing-prototypes -ggdb  
-o parse.o /tools/src/zsh/Src/parse.c
/tools/src/zsh/Src/parse.c: In function `try_dump_file':
/tools/src/zsh/Src/parse.c:2858: `dumps' undeclared (first use in this
function)

/tools/src/zsh/Src/parse.c:2858: (Each undeclared identifier is reported
only once
/tools/src/zsh/Src/parse.c:2858: for each function it appears in.)
/tools/src/zsh/Src/parse.c: In function `check_dump_file':
/tools/src/zsh/Src/parse.c:2938: `dumps' undeclared (first use in this
function)

make[2]: *** [parse.o] Error 1
make[2]: Leaving directory `/tools/build/zsh/Src'
make[1]: *** [modobjs] Error 2
make[1]: Leaving directory `/tools/build/zsh/Src'
make: *** [all] Error 1

dumps is defined under USE_MMAP that is not defined on Cygwin (we do not
support memory mapping here).

-andrej

Re: Cygwin failure RE: PATCH: check deleted .zwc files

[Bart Schaefer]

On Jun 9,  2:05pm, Andrej Borsenkow wrote:
} Subject: Cygwin failure RE: PATCH: check deleted .zwc files
}
} gcc -c -I.  -DHAVE_CONFIG_H -Wall -Wno-implicit -Wmissing-prototypes -ggdb  
} -o parse.o /tools/src/zsh/Src/parse.c
} /tools/src/zsh/Src/parse.c: In function `try_dump_file':
} /tools/src/zsh/Src/parse.c:2858: `dumps' undeclared (first use in this
} function)
} 
} dumps is defined under USE_MMAP that is not defined on Cygwin (we do not
} support memory mapping here).

There's no mention of FuncDump objects anywhere outside parse.c, and no
reason that I can tell for putting zwcstat() into utils.c, and therefore
also no reason to export it.


Index: Src/parse.c
===================================================================
RCS file: /extra/cvsroot/zsh/zsh-4.0/Src/parse.c,v
retrieving revision 1.3
diff -u -r1.3 parse.c
--- parse.c	2001/06/09 17:29:35	1.3
+++ parse.c	2001/06/09 17:38:14
@@ -2775,6 +2775,24 @@
 
 static FuncDump dumps;
 
+/**/
+static int
+zwcstat(char *filename, struct stat *buf, FuncDump dumps)
+{
+    FuncDump f;
+    
+    if (stat(filename, buf)) {
+#ifdef HAVE_FSTAT
+	for (f = dumps; f; f = f->next) {
+	    if (!strncmp(filename, f->filename, strlen(f->filename)) &&
+		!fstat(f->fd, buf))
+		return 0;
+	}
+#endif
+	return 1;
+    } else return 0;
+}
+
 /* Load a dump file (i.e. map it). */
 
 static void
@@ -2829,6 +2847,10 @@
     d->count = 0;
     d->filename = ztrdup(dump);
 }
+
+#else
+
+#define zwcstat(f, b, d) stat(f, b)
 
 #endif
 
Index: Src/utils.c
===================================================================
RCS file: /extra/cvsroot/zsh/zsh-4.0/Src/utils.c,v
retrieving revision 1.5
diff -u -r1.5 utils.c
--- utils.c	2001/06/09 17:29:35	1.5
+++ utils.c	2001/06/09 17:33:41
@@ -3830,22 +3830,3 @@
        return 0;
 }
 #endif
-
-/**/
-mod_export int
-zwcstat(char *filename, struct stat *buf, FuncDump dumps)
-{
-    FuncDump f;
-    
-    if (stat(filename, buf)) {
-#ifdef HAVE_FSTAT
-	for (f = dumps; f; f = f->next) {
-	    if (!strncmp(filename, f->filename, strlen(f->filename)) &&
-		!fstat(f->fd, buf))
-		return 0;
-	}
-#endif
-	return 1;
-    } else return 0;
-    
-}

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: check deleted .zwc files

[Bart Schaefer]

On Jun 8,  3:53pm, Clint Adams wrote:
} 
} > I have a .zwc file for several functions.  I update one of those functions.
} > I then remove and rebuild the .zwc file.  Now, stat() of the .zwc shows
} > that it is newer than the function file, but any running zsh still has a
} > mapped descriptor on the removed file.  With this code:
} 
} > >      if (stat(filename, buf)) {
} [...]
} 
} > zwcstat() will cause zsh to decide that it can keep using the mapped copy,
} > even though the function file is actually newer than the mapped copy.
} 
} Maybe I'm confused.  If you've generated a new .zwc file, that stat()
} will return 0, and the mapped copy won't even be considered.

OK, I was misunderstanding the way the contents of the struct stat are
used after zwcstat() returns them.

If there's a new file on the disk with the name zwcstat() is checking, it
will be stat'd and then check_dump_file() will discover that none of the
mapped files have the same st_dev && st_ino as any previously-loaded file.
This will cause it to map the new file and load the desired function from
it; any already-loaded functions defined in the same file will not be re-
loaded, and the old file will not be unmapped.

So here's the litany of difficulties with this; some are pre-existing, and
some are new with the introduction of zwcstat():

1.  The file search order is violated.  The doc says:

      If ELEMENT already includes a .zwc extension (i.e. the
      extension was explicitly given by the user), ELEMENT is
      searched for the definition of the function without comparing
      its age to that of other files; in fact, there does not need
      to be any directory named ELEMENT without the suffix. ...

    As of 14813, removing the directory's digest file is not sufficient
    to cause the directory to be searched.  This is wrong because there
    is no subsequent st_mtime comparison in this case.

2.  Even st_mtime comparisons to a removed file can be misleading.

      ... the order of searching is, first, in the *parents of*
      directories in fpath for the newer of either a compiled
      directory or a directory in fpath; second, if more than one of
      these contains a definition for the function that is sought, the
      leftmost in the fpath is chosen; and third, within a directory,
      the newer of either a compiled function or an ordinary function
      definition is used.

    Suppose I've `mv'd a file from one directory to another in $fpath;
    the st_mtime of the file is not changed by the `mv' -- in fact, it
    may not even have changed the inode number -- but it should still be
    used in preference to any mapped+removed .zwc, even if it's older
    than the .zwc was at the time it was removed.

3.  If the .zwc file is rewritten in place (so its dev/ino don't change),
    the st_mtime on disk is used to decide whether to continue using
    the mapped copy.

    This is a particularly hairy one [and predates zwstat()].  If the
    .zwc is overwritten, then either (a) the operating system will have
    done some kind of copy-on-write thing and the memory-map will be
    out of date, or (b) the newly-copied data will be accessed via the
    mapped descriptor and zsh may execute garbage.

4.  If the file is mapped, removed, replaced, mapped again, and then
    removed again, etc., there will be multiple entries in the `dumps'
    linked list with the same `filename' field.  The search in zwcstat()
    does not find the most-recently-mapped version of the file.

5.  As Andrej noted, every re-mapping wastes (not exactly leaks) memory,
    though on a system that pages directly out of the mapped file that's
    not of any particular concern.

There's not really much we can do about (3b).  The closest thing would be 
for zcompile to always create .zwc files by unlinking and recreating them
(unless some sort of force option is given), and also to chmod that new
file to remove write permission.

For (1) and (2), it's necessary to split up the test for file existence
from the search for an already-mapped copy, which means we need more than
just zwcstat().

Clint, maybe you could explain more WHY you want this to work?  Is it
for the case where the ONLY place from which zsh can load the function
definition is the (removed but still mapped) digest file?  Or is there
some reason that you want the mapped file to take precedence over any
other file on disk?

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: check deleted .zwc files

[Clint Adams]

> You mean, it will be loaded (mapped) again? Then it is potential (albeit
> slow) memory leak.

It's not really being loaded again, since it's a different file.

Re: PATCH: check deleted .zwc files

[Clint Adams]

> Clint, maybe you could explain more WHY you want this to work?  Is it
> for the case where the ONLY place from which zsh can load the function
> definition is the (removed but still mapped) digest file?  Or is there
> some reason that you want the mapped file to take precedence over any
> other file on disk?

The former.  In Debian, when someone upgrades their zsh package,
the binary, docs, functions, modules, and wordcode digests get
removed, and those of the new version are put in place.
The directories are versioned, so "place" is a unique pathname.
The user with a running shell finds his $fpath emptied,
usually when he hits TAB for a completion that has not yet
been autoloaded, and zsh spams him with pages of "function
definition file not found".

In this particular scenario, the mapped file should never take
precedence over the on-disk file, since unless the administrator
has for some reason reinstalled the very same version, that
on-disk file won't exist.

Perhaps everyone would be happier if the deleted files were only
checked if the function could not be found anywhere else.

Re: PATCH: check deleted .zwc files

[Bart Schaefer]

On Jun 10,  7:34am, Clint Adams wrote:
}
} Perhaps everyone would be happier if the deleted files were only
} checked if the function could not be found anywhere else.

That seems like the most reasonable thing.

It doesn't help with items (3) and (4) on my list, though.  Sven, have
you any thoughts?

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: check deleted .zwc files

[Sven Wischnowsky]

Bart Schaefer wrote:

> On Jun 10,  7:34am, Clint Adams wrote:
> }
> } Perhaps everyone would be happier if the deleted files were only
> } checked if the function could not be found anywhere else.
> 
> That seems like the most reasonable thing.

Yes, I thought the same.

> It doesn't help with items (3) and (4) on my list, though.  Sven, have
> you any thoughts?

For 3. (zwc rewritten in place): I don't see a solution either, but was
thinking about implementing what you suggested (unlinking the file and
chmod()ing it). Should we just do that?

And for 4.: we could `invalidate' the name of an old entry in the list
for the same filename (setting it to the empty string or something). 
But zwcstat() should find the last entry with the same name that was
added to the list because new entries are added in front of it (or am I
missing something?).


All in all I have to admit that I hadn't thought about doing naughty
things like removing or rewriting shared zwc files while they are being
used (I thought that they would be changed only seldom -- which is still
true, I think -- and I thought that for security reasons users would
create their own zwc files and hence could control when they are in use
or not).


Bye
  Sven


-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: check deleted .zwc files

[Bart Schaefer]

On Jun 13, 10:59am, Sven Wischnowsky wrote:
}
} For 3. (zwc rewritten in place): I don't see a solution either, but was
} thinking about implementing what you suggested (unlinking the file and
} chmod()ing it). Should we just do that?

I'd say so, if there are no better ideas.

} But zwcstat() should find the last entry with the same name that was
} added to the list because new entries are added in front of it (or am I
} missing something?).

More probably, I was.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: PATCH: check deleted .zwc files

[Sven Wischnowsky]

Bart Schaefer wrote:

> On Jun 13, 10:59am, Sven Wischnowsky wrote:
> }
> } For 3. (zwc rewritten in place): I don't see a solution either, but was
> } thinking about implementing what you suggested (unlinking the file and
> } chmod()ing it). Should we just do that?
> 
> I'd say so, if there are no better ideas.

Nobody spoke up, so here is the patch that makes zcompile unlink() the
zwc file first and makes it use mode 0444 when creating these files.
Should it use 0400, so that system administrators have to chmod it by
hand?

That hunk in zwcstat() avoids a `unused variable' variable warning if
HAVE_FSTAT is not defined.


Bye
  Sven

Index: Src/parse.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/parse.c,v
retrieving revision 1.22
diff -u -r1.22 parse.c
--- Src/parse.c	2001/06/09 18:06:32	1.22
+++ Src/parse.c	2001/06/18 07:29:58
@@ -2544,7 +2544,8 @@
     if (!strsfx(FD_EXT, dump))
 	dump = dyncat(dump, FD_EXT);
 
-    if ((dfd = open(dump, O_WRONLY|O_CREAT, 0600)) < 0) {
+    unlink(dump);
+    if ((dfd = open(dump, O_WRONLY|O_CREAT, 0444)) < 0) {
 	zwarnnam(nam, "can't write zwc file: %s", dump, 0);
 	return 1;
     }
@@ -2675,7 +2676,8 @@
     if (!strsfx(FD_EXT, dump))
 	dump = dyncat(dump, FD_EXT);
 
-    if ((dfd = open(dump, O_WRONLY|O_CREAT, 0600)) < 0) {
+    unlink(dump);
+    if ((dfd = open(dump, O_WRONLY|O_CREAT, 0444)) < 0) {
 	zwarnnam(nam, "can't write zwc file: %s", dump, 0);
 	return 1;
     }
@@ -2779,10 +2781,10 @@
 static int
 zwcstat(char *filename, struct stat *buf, FuncDump dumps)
 {
-    FuncDump f;
-    
     if (stat(filename, buf)) {
 #ifdef HAVE_FSTAT
+        FuncDump f;
+    
 	for (f = dumps; f; f = f->next) {
 	    if (!strncmp(filename, f->filename, strlen(f->filename)) &&
 		!fstat(f->fd, buf))

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de