diff: plug telldir/seekdir leaks and more

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

diff: plug telldir/seekdir leaks and more

Otto Moerbeek
Hi,

This is a revised version of the diff Paul Thorn
<[hidden email]> send some time ago. It's a mix of
Paul's diff, FreeBSD code and my own:

- plug a huge leak that occurs if telldir() is called, but no
corresponding seekdir(). Samba is suffering from that.
- Use a data structure local to DIR to store the telldir data. FreeBSD
uses a linked list, I chose to use an array, which avoids allocating
lots of small chunks, the index becomes implicit as well, so we can
drop a field from the struct.
- Make sure that loc = telldir(); .... seekdir(loc); telldir() returns loc,
as POSIX requires. This is an area that can be improved, since it now
scans the array.

Note that one documented behaviour is changed. POSIX does not require
it, and most other Unix-like system do not give that guarantee.

Please review and test, especially on setup that uses telldir and
seekdir. AFAIK, no program in base does that, but at least samba
does.

        -Otto

Index: include/dirent.h
===================================================================
RCS file: /cvs/src/include/dirent.h,v
retrieving revision 1.15
diff -u -p -r1.15 dirent.h
--- include/dirent.h 13 Dec 2005 00:35:22 -0000 1.15
+++ include/dirent.h 24 Mar 2006 09:08:19 -0000
@@ -59,6 +59,7 @@
 /* definitions for library routines operating on directories. */
 #define DIRBLKSIZ 1024
 
+struct _telldir;
 /* structure describing an open directory. */
 typedef struct _dirdesc {
  int dd_fd; /* file descriptor associated with directory */
@@ -69,6 +70,7 @@ typedef struct _dirdesc {
  long dd_seek; /* magic cookie returned by getdirentries */
  long dd_rewind; /* magic cookie for rewinding */
  int dd_flags; /* flags for readdir */
+ struct _telldir *dd_td; /* telldir position recording */
 } DIR;
 
 #define dirfd(dirp) ((dirp)->dd_fd)
@@ -106,7 +108,7 @@ int getdirentries(int, char *, int, long
  __attribute__ ((__bounded__(__string__,2,3)));
 #endif /* __BSD_VISIBLE */
 #if __XPG_VISIBLE
-long telldir(const DIR *);
+long telldir(DIR *);
 void seekdir(DIR *, long);
 #endif
 #if __POSIX_VISIBLE >= 199506 || __XPG_VISIBLE >= 500
Index: lib/libc/gen/closedir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/closedir.c,v
retrieving revision 1.6
diff -u -p -r1.6 closedir.c
--- lib/libc/gen/closedir.c 8 Aug 2005 08:05:33 -0000 1.6
+++ lib/libc/gen/closedir.c 24 Mar 2006 09:08:19 -0000
@@ -33,6 +33,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include "thread_private.h"
+#include "telldir.h"
 
 /*
  * close a directory.
@@ -45,12 +46,12 @@ closedir(DIR *dirp)
 
  if ((ret = _FD_LOCK(dirp->dd_fd, FD_READ, NULL)) != 0)
  return (ret);
- seekdir(dirp, dirp->dd_rewind); /* free seekdir storage */
  fd = dirp->dd_fd;
  dirp->dd_fd = -1;
  dirp->dd_loc = 0;
- free((void *)dirp->dd_buf);
- free((void *)dirp);
+ free(dirp->dd_td->td_locs);
+ free(dirp->dd_buf);
+ free(dirp);
  ret = close(fd);
  _FD_UNLOCK(fd, FD_READ);
  return (ret);
Index: lib/libc/gen/directory.3
===================================================================
RCS file: /cvs/src/lib/libc/gen/directory.3,v
retrieving revision 1.16
diff -u -p -r1.16 directory.3
--- lib/libc/gen/directory.3 2 Jun 2003 20:18:34 -0000 1.16
+++ lib/libc/gen/directory.3 24 Mar 2006 09:08:19 -0000
@@ -135,12 +135,6 @@ from which they are derived.
 If the directory is closed and then reopened, the
 .Fn telldir
 value may be invalidated due to undetected directory compaction.
-It is safe to use a previous
-.Fn telldir
-value immediately after a call to
-.Fn opendir
-and before any calls to
-.Fn readdir .
 .Pp
 The
 .Fn rewinddir
Index: lib/libc/gen/opendir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/opendir.c,v
retrieving revision 1.15
diff -u -p -r1.15 opendir.c
--- lib/libc/gen/opendir.c 10 Oct 2005 17:37:43 -0000 1.15
+++ lib/libc/gen/opendir.c 24 Mar 2006 09:08:19 -0000
@@ -40,6 +40,8 @@
 #include <string.h>
 #include <unistd.h>
 
+#include "telldir.h"
+
 /*
  * Open a directory.
  */
@@ -67,10 +69,16 @@ __opendir2(const char *name, int flags)
  return (NULL);
  }
  if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1 ||
-    (dirp = (DIR *)malloc(sizeof(DIR))) == NULL) {
+    (dirp = malloc(sizeof(DIR) + sizeof(struct _telldir))) == NULL) {
  close(fd);
  return (NULL);
  }
+
+ dirp->dd_td = (struct _telldir *)((char *)dirp + sizeof(DIR));
+ dirp->dd_td->td_locs = NULL;
+ dirp->dd_td->td_sz = 0;
+ dirp->dd_td->td_loccnt = 0;
+
 
  /*
  * If the machine's page size is an exact multiple of DIRBLKSIZ,
Index: lib/libc/gen/seekdir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/seekdir.c,v
retrieving revision 1.7
diff -u -p -r1.7 seekdir.c
--- lib/libc/gen/seekdir.c 8 Aug 2005 08:05:34 -0000 1.7
+++ lib/libc/gen/seekdir.c 24 Mar 2006 09:08:19 -0000
@@ -30,8 +30,7 @@
 
 #include <sys/param.h>
 #include <dirent.h>
-
-void __seekdir(DIR *, long);
+#include "telldir.h"
 
 /*
  * Seek to an entry in a directory.
Index: lib/libc/gen/telldir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/telldir.c,v
retrieving revision 1.7
diff -u -p -r1.7 telldir.c
--- lib/libc/gen/telldir.c 8 Aug 2005 08:05:34 -0000 1.7
+++ lib/libc/gen/telldir.c 24 Mar 2006 09:08:19 -0000
@@ -29,56 +29,43 @@
  */
 
 #include <sys/param.h>
+#include <sys/queue.h>
 #include <dirent.h>
 #include <stdlib.h>
 #include <unistd.h>
 
-/*
- * The option SINGLEUSE may be defined to say that a telldir
- * cookie may be used only once before it is freed. This option
- * is used to avoid having memory usage grow without bound.
- */
-#define SINGLEUSE
-
-/*
- * One of these structures is malloced to describe the current directory
- * position each time telldir is called. It records the current magic
- * cookie returned by getdirentries and the offset within the buffer
- * associated with that return value.
- */
-struct ddloc {
- struct ddloc *loc_next;/* next structure in list */
- long loc_index; /* key associated with structure */
- long loc_seek; /* magic cookie returned by getdirentries */
- long loc_loc; /* offset of entry in buffer */
-};
-
-#define NDIRHASH 32 /* Num of hash lists, must be a power of 2 */
-#define LOCHASH(i) ((i)&(NDIRHASH-1))
-
-static long dd_loccnt; /* Index of entry for sequential readdir's */
-static struct ddloc *dd_hash[NDIRHASH];   /* Hash list heads for ddlocs */
-
-void __seekdir(DIR *, long);
+#include "telldir.h"
 
 /*
  * return a pointer into a directory
  */
 long
-telldir(const DIR *dirp)
+telldir(DIR *dirp)
 {
- int index;
- struct ddloc *lp;
+ long i = 0;
+ struct ddloc *lp = dirp->dd_td->td_locs;
+
+ /* return previous telldir, if there */
+ for (; i < dirp->dd_td->td_loccnt; i++, lp++) {
+ if (lp->loc_seek == dirp->dd_seek &&
+    lp->loc_loc == dirp->dd_loc)
+ return (i);
+ }
 
- if ((lp = (struct ddloc *)malloc(sizeof(struct ddloc))) == NULL)
- return (-1);
- index = dd_loccnt++;
- lp->loc_index = index;
+ if (dirp->dd_td->td_loccnt == dirp->dd_td->td_sz) {
+ size_t newsz = dirp->dd_td->td_sz * 2 + 1;
+ struct ddloc *p;
+ p = realloc(dirp->dd_td->td_locs, newsz * sizeof(*p));
+ if (p == NULL)
+ return (-1);
+ dirp->dd_td->td_sz = newsz;
+ dirp->dd_td->td_locs = p;
+ lp = &dirp->dd_td->td_locs[i];
+ }
+ dirp->dd_td->td_loccnt++;
  lp->loc_seek = dirp->dd_seek;
  lp->loc_loc = dirp->dd_loc;
- lp->loc_next = dd_hash[LOCHASH(index)];
- dd_hash[LOCHASH(index)] = lp;
- return (index);
+ return (i);
 }
 
 /*
@@ -89,21 +76,13 @@ void
 __seekdir(DIR *dirp, long loc)
 {
  struct ddloc *lp;
- struct ddloc **prevlp;
  struct dirent *dp;
 
- prevlp = &dd_hash[LOCHASH(loc)];
- lp = *prevlp;
- while (lp != NULL) {
- if (lp->loc_index == loc)
- break;
- prevlp = &lp->loc_next;
- lp = lp->loc_next;
- }
- if (lp == NULL)
+ if (loc < 0 || loc >= dirp->dd_td->td_loccnt)
  return;
+ lp = &dirp->dd_td->td_locs[loc];
  if (lp->loc_loc == dirp->dd_loc && lp->loc_seek == dirp->dd_seek)
- goto found;
+ return;
  (void) lseek(dirp->dd_fd, (off_t)lp->loc_seek, SEEK_SET);
  dirp->dd_seek = lp->loc_seek;
  dirp->dd_loc = 0;
@@ -112,9 +91,4 @@ __seekdir(DIR *dirp, long loc)
  if (dp == NULL)
  break;
  }
-found:
-#ifdef SINGLEUSE
- *prevlp = lp->loc_next;
- free((caddr_t)lp);
-#endif
 }
Index: lib/libc/gen/telldir.h
===================================================================
RCS file: lib/libc/gen/telldir.h
diff -N lib/libc/gen/telldir.h
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ lib/libc/gen/telldir.h 24 Mar 2006 09:08:19 -0000
@@ -0,0 +1,62 @@
+/* $OpenBSD$ */
+/*
+ * Copyright (c) 1983, 1993
+ * The Regents of the University of California.  All rights reserved.
+ *
+ * Copyright (c) 2000
+ * Daniel Eischen.  All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the University nor the names of its contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ *
+ * $FreeBSD: src/lib/libc/gen/telldir.h,v 1.2 2001/01/24 12:59:24 deischen Exp $
+ */
+
+#ifndef _TELLDIR_H_
+#define _TELLDIR_H_
+
+/*
+ * One of these structures is malloced to describe the current directory
+ * position each time telldir is called. It records the current magic
+ * cookie returned by getdirentries and the offset within the buffer
+ * associated with that return value.
+ */
+struct ddloc {
+ long loc_seek; /* magic cookie returned by getdirentries */
+ long loc_loc; /* offset of entry in buffer */
+};
+
+/*
+ * One of these structures is malloced for each DIR to record telldir
+ * positions.
+ */
+struct _telldir {
+ struct ddloc *td_locs; /* locations */
+ size_t td_sz; /* size of locations */
+ long td_loccnt; /* index of entry for sequential readdir's */
+};
+
+void __seekdir(DIR *, long);
+
+#endif

Reply | Threaded
Open this post in threaded view
|

Re: diff: plug telldir/seekdir leaks and more

Otto Moerbeek
On Fri, 24 Mar 2006, Otto Moerbeek wrote:

> Hi,
>
> This is a revised version of the diff Paul Thorn
> <[hidden email]> send some time ago. It's a mix of
> Paul's diff, FreeBSD code and my own:
>
> - plug a huge leak that occurs if telldir() is called, but no
> corresponding seekdir(). Samba is suffering from that.
> - Use a data structure local to DIR to store the telldir data. FreeBSD
> uses a linked list, I chose to use an array, which avoids allocating
> lots of small chunks, the index becomes implicit as well, so we can
> drop a field from the struct.
> - Make sure that loc = telldir(); .... seekdir(loc); telldir() returns loc,
> as POSIX requires. This is an area that can be improved, since it now
> scans the array.
>
> Note that one documented behaviour is changed. POSIX does not require
> it, and most other Unix-like system do not give that guarantee.
>
> Please review and test, especially on setup that uses telldir and
> seekdir. AFAIK, no program in base does that, but at least samba
> does.

Not a single reply to this so far. There must be some samba users
hiding here.

The diff is in snaps as well, to accomodate even easier testing.

I'll crosspost this to misc@ as well, I know it's "not done", but I
really need test reports.

        -Otto

>
> Index: include/dirent.h
> ===================================================================
> RCS file: /cvs/src/include/dirent.h,v
> retrieving revision 1.15
> diff -u -p -r1.15 dirent.h
> --- include/dirent.h 13 Dec 2005 00:35:22 -0000 1.15
> +++ include/dirent.h 24 Mar 2006 09:08:19 -0000
> @@ -59,6 +59,7 @@
>  /* definitions for library routines operating on directories. */
>  #define DIRBLKSIZ 1024
>  
> +struct _telldir;
>  /* structure describing an open directory. */
>  typedef struct _dirdesc {
>   int dd_fd; /* file descriptor associated with directory */
> @@ -69,6 +70,7 @@ typedef struct _dirdesc {
>   long dd_seek; /* magic cookie returned by getdirentries */
>   long dd_rewind; /* magic cookie for rewinding */
>   int dd_flags; /* flags for readdir */
> + struct _telldir *dd_td; /* telldir position recording */
>  } DIR;
>  
>  #define dirfd(dirp) ((dirp)->dd_fd)
> @@ -106,7 +108,7 @@ int getdirentries(int, char *, int, long
>   __attribute__ ((__bounded__(__string__,2,3)));
>  #endif /* __BSD_VISIBLE */
>  #if __XPG_VISIBLE
> -long telldir(const DIR *);
> +long telldir(DIR *);
>  void seekdir(DIR *, long);
>  #endif
>  #if __POSIX_VISIBLE >= 199506 || __XPG_VISIBLE >= 500
> Index: lib/libc/gen/closedir.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/closedir.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 closedir.c
> --- lib/libc/gen/closedir.c 8 Aug 2005 08:05:33 -0000 1.6
> +++ lib/libc/gen/closedir.c 24 Mar 2006 09:08:19 -0000
> @@ -33,6 +33,7 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include "thread_private.h"
> +#include "telldir.h"
>  
>  /*
>   * close a directory.
> @@ -45,12 +46,12 @@ closedir(DIR *dirp)
>  
>   if ((ret = _FD_LOCK(dirp->dd_fd, FD_READ, NULL)) != 0)
>   return (ret);
> - seekdir(dirp, dirp->dd_rewind); /* free seekdir storage */
>   fd = dirp->dd_fd;
>   dirp->dd_fd = -1;
>   dirp->dd_loc = 0;
> - free((void *)dirp->dd_buf);
> - free((void *)dirp);
> + free(dirp->dd_td->td_locs);
> + free(dirp->dd_buf);
> + free(dirp);
>   ret = close(fd);
>   _FD_UNLOCK(fd, FD_READ);
>   return (ret);
> Index: lib/libc/gen/directory.3
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/directory.3,v
> retrieving revision 1.16
> diff -u -p -r1.16 directory.3
> --- lib/libc/gen/directory.3 2 Jun 2003 20:18:34 -0000 1.16
> +++ lib/libc/gen/directory.3 24 Mar 2006 09:08:19 -0000
> @@ -135,12 +135,6 @@ from which they are derived.
>  If the directory is closed and then reopened, the
>  .Fn telldir
>  value may be invalidated due to undetected directory compaction.
> -It is safe to use a previous
> -.Fn telldir
> -value immediately after a call to
> -.Fn opendir
> -and before any calls to
> -.Fn readdir .
>  .Pp
>  The
>  .Fn rewinddir
> Index: lib/libc/gen/opendir.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/opendir.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 opendir.c
> --- lib/libc/gen/opendir.c 10 Oct 2005 17:37:43 -0000 1.15
> +++ lib/libc/gen/opendir.c 24 Mar 2006 09:08:19 -0000
> @@ -40,6 +40,8 @@
>  #include <string.h>
>  #include <unistd.h>
>  
> +#include "telldir.h"
> +
>  /*
>   * Open a directory.
>   */
> @@ -67,10 +69,16 @@ __opendir2(const char *name, int flags)
>   return (NULL);
>   }
>   if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1 ||
> -    (dirp = (DIR *)malloc(sizeof(DIR))) == NULL) {
> +    (dirp = malloc(sizeof(DIR) + sizeof(struct _telldir))) == NULL) {
>   close(fd);
>   return (NULL);
>   }
> +
> + dirp->dd_td = (struct _telldir *)((char *)dirp + sizeof(DIR));
> + dirp->dd_td->td_locs = NULL;
> + dirp->dd_td->td_sz = 0;
> + dirp->dd_td->td_loccnt = 0;
> +
>  
>   /*
>   * If the machine's page size is an exact multiple of DIRBLKSIZ,
> Index: lib/libc/gen/seekdir.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/seekdir.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 seekdir.c
> --- lib/libc/gen/seekdir.c 8 Aug 2005 08:05:34 -0000 1.7
> +++ lib/libc/gen/seekdir.c 24 Mar 2006 09:08:19 -0000
> @@ -30,8 +30,7 @@
>  
>  #include <sys/param.h>
>  #include <dirent.h>
> -
> -void __seekdir(DIR *, long);
> +#include "telldir.h"
>  
>  /*
>   * Seek to an entry in a directory.
> Index: lib/libc/gen/telldir.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/gen/telldir.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 telldir.c
> --- lib/libc/gen/telldir.c 8 Aug 2005 08:05:34 -0000 1.7
> +++ lib/libc/gen/telldir.c 24 Mar 2006 09:08:19 -0000
> @@ -29,56 +29,43 @@
>   */
>  
>  #include <sys/param.h>
> +#include <sys/queue.h>
>  #include <dirent.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  
> -/*
> - * The option SINGLEUSE may be defined to say that a telldir
> - * cookie may be used only once before it is freed. This option
> - * is used to avoid having memory usage grow without bound.
> - */
> -#define SINGLEUSE
> -
> -/*
> - * One of these structures is malloced to describe the current directory
> - * position each time telldir is called. It records the current magic
> - * cookie returned by getdirentries and the offset within the buffer
> - * associated with that return value.
> - */
> -struct ddloc {
> - struct ddloc *loc_next;/* next structure in list */
> - long loc_index; /* key associated with structure */
> - long loc_seek; /* magic cookie returned by getdirentries */
> - long loc_loc; /* offset of entry in buffer */
> -};
> -
> -#define NDIRHASH 32 /* Num of hash lists, must be a power of 2 */
> -#define LOCHASH(i) ((i)&(NDIRHASH-1))
> -
> -static long dd_loccnt; /* Index of entry for sequential readdir's */
> -static struct ddloc *dd_hash[NDIRHASH];   /* Hash list heads for ddlocs */
> -
> -void __seekdir(DIR *, long);
> +#include "telldir.h"
>  
>  /*
>   * return a pointer into a directory
>   */
>  long
> -telldir(const DIR *dirp)
> +telldir(DIR *dirp)
>  {
> - int index;
> - struct ddloc *lp;
> + long i = 0;
> + struct ddloc *lp = dirp->dd_td->td_locs;
> +
> + /* return previous telldir, if there */
> + for (; i < dirp->dd_td->td_loccnt; i++, lp++) {
> + if (lp->loc_seek == dirp->dd_seek &&
> +    lp->loc_loc == dirp->dd_loc)
> + return (i);
> + }
>  
> - if ((lp = (struct ddloc *)malloc(sizeof(struct ddloc))) == NULL)
> - return (-1);
> - index = dd_loccnt++;
> - lp->loc_index = index;
> + if (dirp->dd_td->td_loccnt == dirp->dd_td->td_sz) {
> + size_t newsz = dirp->dd_td->td_sz * 2 + 1;
> + struct ddloc *p;
> + p = realloc(dirp->dd_td->td_locs, newsz * sizeof(*p));
> + if (p == NULL)
> + return (-1);
> + dirp->dd_td->td_sz = newsz;
> + dirp->dd_td->td_locs = p;
> + lp = &dirp->dd_td->td_locs[i];
> + }
> + dirp->dd_td->td_loccnt++;
>   lp->loc_seek = dirp->dd_seek;
>   lp->loc_loc = dirp->dd_loc;
> - lp->loc_next = dd_hash[LOCHASH(index)];
> - dd_hash[LOCHASH(index)] = lp;
> - return (index);
> + return (i);
>  }
>  
>  /*
> @@ -89,21 +76,13 @@ void
>  __seekdir(DIR *dirp, long loc)
>  {
>   struct ddloc *lp;
> - struct ddloc **prevlp;
>   struct dirent *dp;
>  
> - prevlp = &dd_hash[LOCHASH(loc)];
> - lp = *prevlp;
> - while (lp != NULL) {
> - if (lp->loc_index == loc)
> - break;
> - prevlp = &lp->loc_next;
> - lp = lp->loc_next;
> - }
> - if (lp == NULL)
> + if (loc < 0 || loc >= dirp->dd_td->td_loccnt)
>   return;
> + lp = &dirp->dd_td->td_locs[loc];
>   if (lp->loc_loc == dirp->dd_loc && lp->loc_seek == dirp->dd_seek)
> - goto found;
> + return;
>   (void) lseek(dirp->dd_fd, (off_t)lp->loc_seek, SEEK_SET);
>   dirp->dd_seek = lp->loc_seek;
>   dirp->dd_loc = 0;
> @@ -112,9 +91,4 @@ __seekdir(DIR *dirp, long loc)
>   if (dp == NULL)
>   break;
>   }
> -found:
> -#ifdef SINGLEUSE
> - *prevlp = lp->loc_next;
> - free((caddr_t)lp);
> -#endif
>  }
> Index: lib/libc/gen/telldir.h
> ===================================================================
> RCS file: lib/libc/gen/telldir.h
> diff -N lib/libc/gen/telldir.h
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ lib/libc/gen/telldir.h 24 Mar 2006 09:08:19 -0000
> @@ -0,0 +1,62 @@
> +/* $OpenBSD$ */
> +/*
> + * Copyright (c) 1983, 1993
> + * The Regents of the University of California.  All rights reserved.
> + *
> + * Copyright (c) 2000
> + * Daniel Eischen.  All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the University nor the names of its contributors
> + *    may be used to endorse or promote products derived from this software
> + *    without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + *
> + * $FreeBSD: src/lib/libc/gen/telldir.h,v 1.2 2001/01/24 12:59:24 deischen Exp $
> + */
> +
> +#ifndef _TELLDIR_H_
> +#define _TELLDIR_H_
> +
> +/*
> + * One of these structures is malloced to describe the current directory
> + * position each time telldir is called. It records the current magic
> + * cookie returned by getdirentries and the offset within the buffer
> + * associated with that return value.
> + */
> +struct ddloc {
> + long loc_seek; /* magic cookie returned by getdirentries */
> + long loc_loc; /* offset of entry in buffer */
> +};
> +
> +/*
> + * One of these structures is malloced for each DIR to record telldir
> + * positions.
> + */
> +struct _telldir {
> + struct ddloc *td_locs; /* locations */
> + size_t td_sz; /* size of locations */
> + long td_loccnt; /* index of entry for sequential readdir's */
> +};
> +
> +void __seekdir(DIR *, long);
> +
> +#endif

Reply | Threaded
Open this post in threaded view
|

Re: diff: plug telldir/seekdir leaks and more

Steve Williams-2
Just as a FYI..

I follow the Samba tech list, and they have replacement
opendir,telldir,seekdir,closedir calls in their source tree.

They have modified Samba to use their own equivalent calls instead of
relying on the calls in OpebBSD.

I don't have a SVN commit, but here's Jeremy's comment...

The subject of the thread is "Samba v3.0.21c"

> On Mon, Mar 27, 2006 at 06:54:03PM +1100, [hidden email] wrote:
>  
>> > Jeremy,
>> >
>> >  > There is a "feature" in the particular sequence of events with an
>> >  > opendir, seekdir, telldir, system calls that can cause memory usage
>> >  > increase.  You should be able to find it on the mail list archives.
>> >
>> > It might be an idea to merge across the lib/replace/repdir/
>> > replacements for opendir and related calls from Samba4 to Samba3 to
>> > fix this. The BSD ones are too broken to use, and it seems simplest to
>> > just replace them on the broken systems.
>> >
>> > See also the autoconf test in lib/replace/repdir/config.m4
>>    
>
> Ok - given it a go in SAMBA_3_0 and trunk. Now if the *BSD'ers
> could give it a try....
>
> Jeremy.
>  

Otto Moerbeek wrote:

> On Fri, 24 Mar 2006, Otto Moerbeek wrote:
>
>  
>> Hi,
>>
>> This is a revised version of the diff Paul Thorn
>> <[hidden email]> send some time ago. It's a mix of
>> Paul's diff, FreeBSD code and my own:
>>
>> - plug a huge leak that occurs if telldir() is called, but no
>> corresponding seekdir(). Samba is suffering from that.
>> - Use a data structure local to DIR to store the telldir data. FreeBSD
>> uses a linked list, I chose to use an array, which avoids allocating
>> lots of small chunks, the index becomes implicit as well, so we can
>> drop a field from the struct.
>> - Make sure that loc = telldir(); .... seekdir(loc); telldir() returns loc,
>> as POSIX requires. This is an area that can be improved, since it now
>> scans the array.
>>
>> Note that one documented behaviour is changed. POSIX does not require
>> it, and most other Unix-like system do not give that guarantee.
>>
>> Please review and test, especially on setup that uses telldir and
>> seekdir. AFAIK, no program in base does that, but at least samba
>> does.
>>    
>
> Not a single reply to this so far. There must be some samba users
> hiding here.
>
> The diff is in snaps as well, to accomodate even easier testing.
>
> I'll crosspost this to misc@ as well, I know it's "not done", but I
> really need test reports.
>
> -Otto
>  
>> Index: include/dirent.h
>> ===================================================================
>> RCS file: /cvs/src/include/dirent.h,v
>> retrieving revision 1.15
>> diff -u -p -r1.15 dirent.h
>> --- include/dirent.h 13 Dec 2005 00:35:22 -0000 1.15
>> +++ include/dirent.h 24 Mar 2006 09:08:19 -0000
>> @@ -59,6 +59,7 @@
>>  /* definitions for library routines operating on directories. */
>>  #define DIRBLKSIZ 1024
>>  
>> +struct _telldir;
>>  /* structure describing an open directory. */
>>  typedef struct _dirdesc {
>>   int dd_fd; /* file descriptor associated with directory */
>> @@ -69,6 +70,7 @@ typedef struct _dirdesc {
>>   long dd_seek; /* magic cookie returned by getdirentries */
>>   long dd_rewind; /* magic cookie for rewinding */
>>   int dd_flags; /* flags for readdir */
>> + struct _telldir *dd_td; /* telldir position recording */
>>  } DIR;
>>  
>>  #define dirfd(dirp) ((dirp)->dd_fd)
>> @@ -106,7 +108,7 @@ int getdirentries(int, char *, int, long
>>   __attribute__ ((__bounded__(__string__,2,3)));
>>  #endif /* __BSD_VISIBLE */
>>  #if __XPG_VISIBLE
>> -long telldir(const DIR *);
>> +long telldir(DIR *);
>>  void seekdir(DIR *, long);
>>  #endif
>>  #if __POSIX_VISIBLE >= 199506 || __XPG_VISIBLE >= 500
>> Index: lib/libc/gen/closedir.c
>> ===================================================================
>> RCS file: /cvs/src/lib/libc/gen/closedir.c,v
>> retrieving revision 1.6
>> diff -u -p -r1.6 closedir.c
>> --- lib/libc/gen/closedir.c 8 Aug 2005 08:05:33 -0000 1.6
>> +++ lib/libc/gen/closedir.c 24 Mar 2006 09:08:19 -0000
>> @@ -33,6 +33,7 @@
>>  #include <stdlib.h>
>>  #include <unistd.h>
>>  #include "thread_private.h"
>> +#include "telldir.h"
>>  
>>  /*
>>   * close a directory.
>> @@ -45,12 +46,12 @@ closedir(DIR *dirp)
>>  
>>   if ((ret = _FD_LOCK(dirp->dd_fd, FD_READ, NULL)) != 0)
>>   return (ret);
>> - seekdir(dirp, dirp->dd_rewind); /* free seekdir storage */
>>   fd = dirp->dd_fd;
>>   dirp->dd_fd = -1;
>>   dirp->dd_loc = 0;
>> - free((void *)dirp->dd_buf);
>> - free((void *)dirp);
>> + free(dirp->dd_td->td_locs);
>> + free(dirp->dd_buf);
>> + free(dirp);
>>   ret = close(fd);
>>   _FD_UNLOCK(fd, FD_READ);
>>   return (ret);
>> Index: lib/libc/gen/directory.3
>> ===================================================================
>> RCS file: /cvs/src/lib/libc/gen/directory.3,v
>> retrieving revision 1.16
>> diff -u -p -r1.16 directory.3
>> --- lib/libc/gen/directory.3 2 Jun 2003 20:18:34 -0000 1.16
>> +++ lib/libc/gen/directory.3 24 Mar 2006 09:08:19 -0000
>> @@ -135,12 +135,6 @@ from which they are derived.
>>  If the directory is closed and then reopened, the
>>  .Fn telldir
>>  value may be invalidated due to undetected directory compaction.
>> -It is safe to use a previous
>> -.Fn telldir
>> -value immediately after a call to
>> -.Fn opendir
>> -and before any calls to
>> -.Fn readdir .
>>  .Pp
>>  The
>>  .Fn rewinddir
>> Index: lib/libc/gen/opendir.c
>> ===================================================================
>> RCS file: /cvs/src/lib/libc/gen/opendir.c,v
>> retrieving revision 1.15
>> diff -u -p -r1.15 opendir.c
>> --- lib/libc/gen/opendir.c 10 Oct 2005 17:37:43 -0000 1.15
>> +++ lib/libc/gen/opendir.c 24 Mar 2006 09:08:19 -0000
>> @@ -40,6 +40,8 @@
>>  #include <string.h>
>>  #include <unistd.h>
>>  
>> +#include "telldir.h"
>> +
>>  /*
>>   * Open a directory.
>>   */
>> @@ -67,10 +69,16 @@ __opendir2(const char *name, int flags)
>>   return (NULL);
>>   }
>>   if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1 ||
>> -    (dirp = (DIR *)malloc(sizeof(DIR))) == NULL) {
>> +    (dirp = malloc(sizeof(DIR) + sizeof(struct _telldir))) == NULL) {
>>   close(fd);
>>   return (NULL);
>>   }
>> +
>> + dirp->dd_td = (struct _telldir *)((char *)dirp + sizeof(DIR));
>> + dirp->dd_td->td_locs = NULL;
>> + dirp->dd_td->td_sz = 0;
>> + dirp->dd_td->td_loccnt = 0;
>> +
>>  
>>   /*
>>   * If the machine's page size is an exact multiple of DIRBLKSIZ,
>> Index: lib/libc/gen/seekdir.c
>> ===================================================================
>> RCS file: /cvs/src/lib/libc/gen/seekdir.c,v
>> retrieving revision 1.7
>> diff -u -p -r1.7 seekdir.c
>> --- lib/libc/gen/seekdir.c 8 Aug 2005 08:05:34 -0000 1.7
>> +++ lib/libc/gen/seekdir.c 24 Mar 2006 09:08:19 -0000
>> @@ -30,8 +30,7 @@
>>  
>>  #include <sys/param.h>
>>  #include <dirent.h>
>> -
>> -void __seekdir(DIR *, long);
>> +#include "telldir.h"
>>  
>>  /*
>>   * Seek to an entry in a directory.
>> Index: lib/libc/gen/telldir.c
>> ===================================================================
>> RCS file: /cvs/src/lib/libc/gen/telldir.c,v
>> retrieving revision 1.7
>> diff -u -p -r1.7 telldir.c
>> --- lib/libc/gen/telldir.c 8 Aug 2005 08:05:34 -0000 1.7
>> +++ lib/libc/gen/telldir.c 24 Mar 2006 09:08:19 -0000
>> @@ -29,56 +29,43 @@
>>   */
>>  
>>  #include <sys/param.h>
>> +#include <sys/queue.h>
>>  #include <dirent.h>
>>  #include <stdlib.h>
>>  #include <unistd.h>
>>  
>> -/*
>> - * The option SINGLEUSE may be defined to say that a telldir
>> - * cookie may be used only once before it is freed. This option
>> - * is used to avoid having memory usage grow without bound.
>> - */
>> -#define SINGLEUSE
>> -
>> -/*
>> - * One of these structures is malloced to describe the current directory
>> - * position each time telldir is called. It records the current magic
>> - * cookie returned by getdirentries and the offset within the buffer
>> - * associated with that return value.
>> - */
>> -struct ddloc {
>> - struct ddloc *loc_next;/* next structure in list */
>> - long loc_index; /* key associated with structure */
>> - long loc_seek; /* magic cookie returned by getdirentries */
>> - long loc_loc; /* offset of entry in buffer */
>> -};
>> -
>> -#define NDIRHASH 32 /* Num of hash lists, must be a power of 2 */
>> -#define LOCHASH(i) ((i)&(NDIRHASH-1))
>> -
>> -static long dd_loccnt; /* Index of entry for sequential readdir's */
>> -static struct ddloc *dd_hash[NDIRHASH];   /* Hash list heads for ddlocs */
>> -
>> -void __seekdir(DIR *, long);
>> +#include "telldir.h"
>>  
>>  /*
>>   * return a pointer into a directory
>>   */
>>  long
>> -telldir(const DIR *dirp)
>> +telldir(DIR *dirp)
>>  {
>> - int index;
>> - struct ddloc *lp;
>> + long i = 0;
>> + struct ddloc *lp = dirp->dd_td->td_locs;
>> +
>> + /* return previous telldir, if there */
>> + for (; i < dirp->dd_td->td_loccnt; i++, lp++) {
>> + if (lp->loc_seek == dirp->dd_seek &&
>> +    lp->loc_loc == dirp->dd_loc)
>> + return (i);
>> + }
>>  
>> - if ((lp = (struct ddloc *)malloc(sizeof(struct ddloc))) == NULL)
>> - return (-1);
>> - index = dd_loccnt++;
>> - lp->loc_index = index;
>> + if (dirp->dd_td->td_loccnt == dirp->dd_td->td_sz) {
>> + size_t newsz = dirp->dd_td->td_sz * 2 + 1;
>> + struct ddloc *p;
>> + p = realloc(dirp->dd_td->td_locs, newsz * sizeof(*p));
>> + if (p == NULL)
>> + return (-1);
>> + dirp->dd_td->td_sz = newsz;
>> + dirp->dd_td->td_locs = p;
>> + lp = &dirp->dd_td->td_locs[i];
>> + }
>> + dirp->dd_td->td_loccnt++;
>>   lp->loc_seek = dirp->dd_seek;
>>   lp->loc_loc = dirp->dd_loc;
>> - lp->loc_next = dd_hash[LOCHASH(index)];
>> - dd_hash[LOCHASH(index)] = lp;
>> - return (index);
>> + return (i);
>>  }
>>  
>>  /*
>> @@ -89,21 +76,13 @@ void
>>  __seekdir(DIR *dirp, long loc)
>>  {
>>   struct ddloc *lp;
>> - struct ddloc **prevlp;
>>   struct dirent *dp;
>>  
>> - prevlp = &dd_hash[LOCHASH(loc)];
>> - lp = *prevlp;
>> - while (lp != NULL) {
>> - if (lp->loc_index == loc)
>> - break;
>> - prevlp = &lp->loc_next;
>> - lp = lp->loc_next;
>> - }
>> - if (lp == NULL)
>> + if (loc < 0 || loc >= dirp->dd_td->td_loccnt)
>>   return;
>> + lp = &dirp->dd_td->td_locs[loc];
>>   if (lp->loc_loc == dirp->dd_loc && lp->loc_seek == dirp->dd_seek)
>> - goto found;
>> + return;
>>   (void) lseek(dirp->dd_fd, (off_t)lp->loc_seek, SEEK_SET);
>>   dirp->dd_seek = lp->loc_seek;
>>   dirp->dd_loc = 0;
>> @@ -112,9 +91,4 @@ __seekdir(DIR *dirp, long loc)
>>   if (dp == NULL)
>>   break;
>>   }
>> -found:
>> -#ifdef SINGLEUSE
>> - *prevlp = lp->loc_next;
>> - free((caddr_t)lp);
>> -#endif
>>  }
>> Index: lib/libc/gen/telldir.h
>> ===================================================================
>> RCS file: lib/libc/gen/telldir.h
>> diff -N lib/libc/gen/telldir.h
>> --- /dev/null 1 Jan 1970 00:00:00 -0000
>> +++ lib/libc/gen/telldir.h 24 Mar 2006 09:08:19 -0000
>> @@ -0,0 +1,62 @@
>> +/* $OpenBSD$ */
>> +/*
>> + * Copyright (c) 1983, 1993
>> + * The Regents of the University of California.  All rights reserved.
>> + *
>> + * Copyright (c) 2000
>> + * Daniel Eischen.  All rights reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + * 1. Redistributions of source code must retain the above copyright
>> + *    notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + *    notice, this list of conditions and the following disclaimer in the
>> + *    documentation and/or other materials provided with the distribution.
>> + * 3. Neither the name of the University nor the names of its contributors
>> + *    may be used to endorse or promote products derived from this software
>> + *    without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
>> + * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
>> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
>> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
>> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
>> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> + * SUCH DAMAGE.
>> + *
>> + * $FreeBSD: src/lib/libc/gen/telldir.h,v 1.2 2001/01/24 12:59:24 deischen Exp $
>> + */
>> +
>> +#ifndef _TELLDIR_H_
>> +#define _TELLDIR_H_
>> +
>> +/*
>> + * One of these structures is malloced to describe the current directory
>> + * position each time telldir is called. It records the current magic
>> + * cookie returned by getdirentries and the offset within the buffer
>> + * associated with that return value.
>> + */
>> +struct ddloc {
>> + long loc_seek; /* magic cookie returned by getdirentries */
>> + long loc_loc; /* offset of entry in buffer */
>> +};
>> +
>> +/*
>> + * One of these structures is malloced for each DIR to record telldir
>> + * positions.
>> + */
>> +struct _telldir {
>> + struct ddloc *td_locs; /* locations */
>> + size_t td_sz; /* size of locations */
>> + long td_loccnt; /* index of entry for sequential readdir's */
>> +};
>> +
>> +void __seekdir(DIR *, long);
>> +
>> +#endif

Reply | Threaded
Open this post in threaded view
|

Re: diff: plug telldir/seekdir leaks and more

Otto Moerbeek
On Wed, 29 Mar 2006, Steve Williams wrote:

> Just as a FYI..

Thanks. But even then, it makes sense to repair them,

        -Otto

>
> I follow the Samba tech list, and they have replacement
> opendir,telldir,seekdir,closedir calls in their source tree.
>
> They have modified Samba to use their own equivalent calls instead of relying
> on the calls in OpebBSD.
>
> I don't have a SVN commit, but here's Jeremy's comment...
>
> The subject of the thread is "Samba v3.0.21c"
>
> > On Mon, Mar 27, 2006 at 06:54:03PM +1100, [hidden email] wrote:
> >  
> > > > Jeremy,
> > > > >  > There is a "feature" in the particular sequence of events with an >
> > > > opendir, seekdir, telldir, system calls that can cause memory usage >  >
> > > increase.  You should be able to find it on the mail list archives.
> > > > > It might be an idea to merge across the lib/replace/repdir/
> > > > replacements for opendir and related calls from Samba4 to Samba3 to
> > > > fix this. The BSD ones are too broken to use, and it seems simplest to
> > > > just replace them on the broken systems.
> > > > > See also the autoconf test in lib/replace/repdir/config.m4
> > >    
> >
> > Ok - given it a go in SAMBA_3_0 and trunk. Now if the *BSD'ers
> > could give it a try....
> >
> > Jeremy.
> >  
>
> Otto Moerbeek wrote:
> > On Fri, 24 Mar 2006, Otto Moerbeek wrote:
> >
> >  
> > > Hi,
> > >
> > > This is a revised version of the diff Paul Thorn
> > > <[hidden email]> send some time ago. It's a mix of
> > > Paul's diff, FreeBSD code and my own:
> > >
> > > - plug a huge leak that occurs if telldir() is called, but no
> > > corresponding seekdir(). Samba is suffering from that. - Use a data
> > > structure local to DIR to store the telldir data. FreeBSD
> > > uses a linked list, I chose to use an array, which avoids allocating
> > > lots of small chunks, the index becomes implicit as well, so we can
> > > drop a field from the struct.
> > > - Make sure that loc = telldir(); .... seekdir(loc); telldir() returns
> > > loc,
> > > as POSIX requires. This is an area that can be improved, since it now
> > > scans the array.
> > >
> > > Note that one documented behaviour is changed. POSIX does not require
> > > it, and most other Unix-like system do not give that guarantee.
> > >
> > > Please review and test, especially on setup that uses telldir and
> > > seekdir. AFAIK, no program in base does that, but at least samba
> > > does.
> > >    
> >
> > Not a single reply to this so far. There must be some samba users
> > hiding here.
> > The diff is in snaps as well, to accomodate even easier testing.
> >
> > I'll crosspost this to misc@ as well, I know it's "not done", but I
> > really need test reports.
> >
> > -Otto
> >  
> > > Index: include/dirent.h
> > > ===================================================================
> > > RCS file: /cvs/src/include/dirent.h,v
> > > retrieving revision 1.15
> > > diff -u -p -r1.15 dirent.h
> > > --- include/dirent.h 13 Dec 2005 00:35:22 -0000 1.15
> > > +++ include/dirent.h 24 Mar 2006 09:08:19 -0000
> > > @@ -59,6 +59,7 @@
> > >  /* definitions for library routines operating on directories. */
> > >  #define DIRBLKSIZ 1024
> > >  +struct _telldir;
> > >  /* structure describing an open directory. */
> > >  typedef struct _dirdesc {
> > >   int dd_fd; /* file descriptor associated with directory
> > > */
> > > @@ -69,6 +70,7 @@ typedef struct _dirdesc {
> > >   long dd_seek; /* magic cookie returned by getdirentries */
> > >   long dd_rewind; /* magic cookie for rewinding */
> > >   int dd_flags; /* flags for readdir */
> > > + struct _telldir *dd_td; /* telldir position recording */
> > >  } DIR;
> > >   #define dirfd(dirp) ((dirp)->dd_fd)
> > > @@ -106,7 +108,7 @@ int getdirentries(int, char *, int, long
> > >   __attribute__ ((__bounded__(__string__,2,3)));
> > >  #endif /* __BSD_VISIBLE */
> > >  #if __XPG_VISIBLE
> > > -long telldir(const DIR *);
> > > +long telldir(DIR *);
> > >  void seekdir(DIR *, long);
> > >  #endif
> > >  #if __POSIX_VISIBLE >= 199506 || __XPG_VISIBLE >= 500
> > > Index: lib/libc/gen/closedir.c
> > > ===================================================================
> > > RCS file: /cvs/src/lib/libc/gen/closedir.c,v
> > > retrieving revision 1.6
> > > diff -u -p -r1.6 closedir.c
> > > --- lib/libc/gen/closedir.c 8 Aug 2005 08:05:33 -0000 1.6
> > > +++ lib/libc/gen/closedir.c 24 Mar 2006 09:08:19 -0000
> > > @@ -33,6 +33,7 @@
> > >  #include <stdlib.h>
> > >  #include <unistd.h>
> > >  #include "thread_private.h"
> > > +#include "telldir.h"
> > >   /*
> > >   * close a directory.
> > > @@ -45,12 +46,12 @@ closedir(DIR *dirp)
> > >   if ((ret = _FD_LOCK(dirp->dd_fd, FD_READ, NULL)) != 0)
> > >   return (ret);
> > > - seekdir(dirp, dirp->dd_rewind); /* free seekdir storage */
> > >   fd = dirp->dd_fd;
> > >   dirp->dd_fd = -1;
> > >   dirp->dd_loc = 0;
> > > - free((void *)dirp->dd_buf);
> > > - free((void *)dirp);
> > > + free(dirp->dd_td->td_locs);
> > > + free(dirp->dd_buf);
> > > + free(dirp);
> > >   ret = close(fd);
> > >   _FD_UNLOCK(fd, FD_READ);
> > >   return (ret);
> > > Index: lib/libc/gen/directory.3
> > > ===================================================================
> > > RCS file: /cvs/src/lib/libc/gen/directory.3,v
> > > retrieving revision 1.16
> > > diff -u -p -r1.16 directory.3
> > > --- lib/libc/gen/directory.3 2 Jun 2003 20:18:34 -0000 1.16
> > > +++ lib/libc/gen/directory.3 24 Mar 2006 09:08:19 -0000
> > > @@ -135,12 +135,6 @@ from which they are derived.
> > >  If the directory is closed and then reopened, the
> > >  .Fn telldir
> > >  value may be invalidated due to undetected directory compaction.
> > > -It is safe to use a previous
> > > -.Fn telldir
> > > -value immediately after a call to
> > > -.Fn opendir
> > > -and before any calls to
> > > -.Fn readdir .
> > >  .Pp
> > >  The
> > >  .Fn rewinddir
> > > Index: lib/libc/gen/opendir.c
> > > ===================================================================
> > > RCS file: /cvs/src/lib/libc/gen/opendir.c,v
> > > retrieving revision 1.15
> > > diff -u -p -r1.15 opendir.c
> > > --- lib/libc/gen/opendir.c 10 Oct 2005 17:37:43 -0000 1.15
> > > +++ lib/libc/gen/opendir.c 24 Mar 2006 09:08:19 -0000
> > > @@ -40,6 +40,8 @@
> > >  #include <string.h>
> > >  #include <unistd.h>
> > >  +#include "telldir.h"
> > > +
> > >  /*
> > >   * Open a directory.
> > >   */
> > > @@ -67,10 +69,16 @@ __opendir2(const char *name, int flags)
> > >   return (NULL);
> > >   }
> > >   if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1 ||
> > > -    (dirp = (DIR *)malloc(sizeof(DIR))) == NULL) {
> > > +    (dirp = malloc(sizeof(DIR) + sizeof(struct _telldir))) == NULL) {
> > >   close(fd);
> > >   return (NULL);
> > >   }
> > > +
> > > + dirp->dd_td = (struct _telldir *)((char *)dirp + sizeof(DIR));
> > > + dirp->dd_td->td_locs = NULL;
> > > + dirp->dd_td->td_sz = 0;
> > > + dirp->dd_td->td_loccnt = 0;
> > > +
> > >   /*
> > >   * If the machine's page size is an exact multiple of DIRBLKSIZ,
> > > Index: lib/libc/gen/seekdir.c
> > > ===================================================================
> > > RCS file: /cvs/src/lib/libc/gen/seekdir.c,v
> > > retrieving revision 1.7
> > > diff -u -p -r1.7 seekdir.c
> > > --- lib/libc/gen/seekdir.c 8 Aug 2005 08:05:34 -0000 1.7
> > > +++ lib/libc/gen/seekdir.c 24 Mar 2006 09:08:19 -0000
> > > @@ -30,8 +30,7 @@
> > >   #include <sys/param.h>
> > >  #include <dirent.h>
> > > -
> > > -void __seekdir(DIR *, long);
> > > +#include "telldir.h"
> > >   /*
> > >   * Seek to an entry in a directory.
> > > Index: lib/libc/gen/telldir.c
> > > ===================================================================
> > > RCS file: /cvs/src/lib/libc/gen/telldir.c,v
> > > retrieving revision 1.7
> > > diff -u -p -r1.7 telldir.c
> > > --- lib/libc/gen/telldir.c 8 Aug 2005 08:05:34 -0000 1.7
> > > +++ lib/libc/gen/telldir.c 24 Mar 2006 09:08:19 -0000
> > > @@ -29,56 +29,43 @@
> > >   */
> > >   #include <sys/param.h>
> > > +#include <sys/queue.h>
> > >  #include <dirent.h>
> > >  #include <stdlib.h>
> > >  #include <unistd.h>
> > >  -/*
> > > - * The option SINGLEUSE may be defined to say that a telldir
> > > - * cookie may be used only once before it is freed. This option
> > > - * is used to avoid having memory usage grow without bound.
> > > - */
> > > -#define SINGLEUSE
> > > -
> > > -/*
> > > - * One of these structures is malloced to describe the current directory
> > > - * position each time telldir is called. It records the current magic - *
> > > cookie returned by getdirentries and the offset within the buffer
> > > - * associated with that return value.
> > > - */
> > > -struct ddloc {
> > > - struct ddloc *loc_next;/* next structure in list */
> > > - long loc_index; /* key associated with structure */
> > > - long loc_seek; /* magic cookie returned by getdirentries */
> > > - long loc_loc; /* offset of entry in buffer */
> > > -};
> > > -
> > > -#define NDIRHASH 32 /* Num of hash lists, must be a power
> > > of 2 */
> > > -#define LOCHASH(i) ((i)&(NDIRHASH-1))
> > > -
> > > -static long dd_loccnt; /* Index of entry for sequential
> > > readdir's */
> > > -static struct ddloc *dd_hash[NDIRHASH];   /* Hash list heads for
> > > ddlocs */
> > > -
> > > -void __seekdir(DIR *, long);
> > > +#include "telldir.h"
> > >   /*
> > >   * return a pointer into a directory
> > >   */
> > >  long
> > > -telldir(const DIR *dirp)
> > > +telldir(DIR *dirp)
> > >  {
> > > - int index;
> > > - struct ddloc *lp;
> > > + long i = 0;
> > > + struct ddloc *lp = dirp->dd_td->td_locs;
> > > +
> > > + /* return previous telldir, if there */
> > > + for (; i < dirp->dd_td->td_loccnt; i++, lp++) {
> > > + if (lp->loc_seek == dirp->dd_seek && +
> > > lp->loc_loc == dirp->dd_loc)
> > > + return (i);
> > > + }
> > >  - if ((lp = (struct ddloc *)malloc(sizeof(struct ddloc))) == NULL)
> > > - return (-1);
> > > - index = dd_loccnt++;
> > > - lp->loc_index = index;
> > > + if (dirp->dd_td->td_loccnt == dirp->dd_td->td_sz) {
> > > + size_t newsz = dirp->dd_td->td_sz * 2 + 1;
> > > + struct ddloc *p;
> > > + p = realloc(dirp->dd_td->td_locs, newsz * sizeof(*p));
> > > + if (p == NULL)
> > > + return (-1);
> > > + dirp->dd_td->td_sz = newsz;
> > > + dirp->dd_td->td_locs = p;
> > > + lp = &dirp->dd_td->td_locs[i];
> > > + }
> > > + dirp->dd_td->td_loccnt++;
> > >   lp->loc_seek = dirp->dd_seek;
> > >   lp->loc_loc = dirp->dd_loc;
> > > - lp->loc_next = dd_hash[LOCHASH(index)];
> > > - dd_hash[LOCHASH(index)] = lp;
> > > - return (index);
> > > + return (i);
> > >  }
> > >   /*
> > > @@ -89,21 +76,13 @@ void
> > >  __seekdir(DIR *dirp, long loc)
> > >  {
> > >   struct ddloc *lp;
> > > - struct ddloc **prevlp;
> > >   struct dirent *dp;
> > >  - prevlp = &dd_hash[LOCHASH(loc)];
> > > - lp = *prevlp;
> > > - while (lp != NULL) {
> > > - if (lp->loc_index == loc)
> > > - break;
> > > - prevlp = &lp->loc_next;
> > > - lp = lp->loc_next;
> > > - }
> > > - if (lp == NULL)
> > > + if (loc < 0 || loc >= dirp->dd_td->td_loccnt)
> > >   return;
> > > + lp = &dirp->dd_td->td_locs[loc];
> > >   if (lp->loc_loc == dirp->dd_loc && lp->loc_seek == dirp->dd_seek)
> > > - goto found;
> > > + return;
> > >   (void) lseek(dirp->dd_fd, (off_t)lp->loc_seek, SEEK_SET);
> > >   dirp->dd_seek = lp->loc_seek;
> > >   dirp->dd_loc = 0;
> > > @@ -112,9 +91,4 @@ __seekdir(DIR *dirp, long loc)
> > >   if (dp == NULL)
> > >   break;
> > >   }
> > > -found:
> > > -#ifdef SINGLEUSE
> > > - *prevlp = lp->loc_next;
> > > - free((caddr_t)lp);
> > > -#endif
> > >  }
> > > Index: lib/libc/gen/telldir.h
> > > ===================================================================
> > > RCS file: lib/libc/gen/telldir.h
> > > diff -N lib/libc/gen/telldir.h
> > > --- /dev/null 1 Jan 1970 00:00:00 -0000
> > > +++ lib/libc/gen/telldir.h 24 Mar 2006 09:08:19 -0000
> > > @@ -0,0 +1,62 @@
> > > +/* $OpenBSD$ */
> > > +/*
> > > + * Copyright (c) 1983, 1993
> > > + * The Regents of the University of California.  All rights reserved.
> > > + *
> > > + * Copyright (c) 2000
> > > + * Daniel Eischen.  All rights reserved.
> > > + *
> > > + * Redistribution and use in source and binary forms, with or without
> > > + * modification, are permitted provided that the following conditions
> > > + * are met:
> > > + * 1. Redistributions of source code must retain the above copyright
> > > + *    notice, this list of conditions and the following disclaimer.
> > > + * 2. Redistributions in binary form must reproduce the above copyright
> > > + *    notice, this list of conditions and the following disclaimer in the
> > > + *    documentation and/or other materials provided with the
> > > distribution.
> > > + * 3. Neither the name of the University nor the names of its
> > > contributors
> > > + *    may be used to endorse or promote products derived from this
> > > software
> > > + *    without specific prior written permission.
> > > + *
> > > + * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS''
> > > AND
> > > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> > > PURPOSE
> > > + * ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE
> > > LIABLE
> > > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > > CONSEQUENTIAL
> > > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE
> > > GOODS
> > > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> > > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> > > STRICT
> > > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
> > > WAY
> > > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > > + * SUCH DAMAGE.
> > > + *
> > > + * $FreeBSD: src/lib/libc/gen/telldir.h,v 1.2 2001/01/24 12:59:24
> > > deischen Exp $
> > > + */
> > > +
> > > +#ifndef _TELLDIR_H_
> > > +#define _TELLDIR_H_
> > > +
> > > +/*
> > > + * One of these structures is malloced to describe the current directory
> > > + * position each time telldir is called. It records the current magic
> > > + * cookie returned by getdirentries and the offset within the buffer
> > > + * associated with that return value.
> > > + */
> > > +struct ddloc {
> > > + long loc_seek; /* magic cookie returned by getdirentries */
> > > + long loc_loc; /* offset of entry in buffer */
> > > +};
> > > +
> > > +/*
> > > + * One of these structures is malloced for each DIR to record telldir
> > > + * positions.
> > > + */
> > > +struct _telldir {
> > > + struct ddloc *td_locs; /* locations */
> > > + size_t td_sz; /* size of locations */
> > > + long td_loccnt; /* index of entry for sequential readdir's */
> > > +};
> > > +
> > > +void __seekdir(DIR *, long);
> > > +
> > > +#endif

Reply | Threaded
Open this post in threaded view
|

Re: diff: plug telldir/seekdir leaks and more

OpenBSD-9
In reply to this post by Otto Moerbeek
Otto Moerbeek wrote:

>
>
> Not a single reply to this so far. There must be some samba users
> hiding here.
>
> The diff is in snaps as well, to accomodate even easier testing.
>
> I'll crosspost this to misc@ as well, I know it's "not done", but I
> really need test reports.
>
> -Otto
>

Hi Otto,

Just installed the snapshots and samba, and everything appears to be
fine copying files backwards and forwards between the samba server and a
Windows 2000 box, reading and writing files, playing media and renaming,
moving and deleting all seems okay.

Not sure what to look for in terms of bugs...

The following gives the output of uname, testparm, smbd --version and
dmesg, and a screen scrap of top during a large file transfer, if that
is of any help.

Thanks

Fred
--
http://www.bristolshotokan.org.uk/
--

Script started on Wed Mar 29 23:22:04 2006
thor:fred /home/fred> uname -a
OpenBSD thor.crowsons.net 3.9 GENERIC#662 i386
thor:fred /home/fred> /usr/local/libecxec/smbd --version
Version 3.0.21b
thor:fred /home/fred> testparm
Load smb config files from /etc/samba/smb.conf
Processing section "[homes]"
Processing section "[printers]"
Processing section "[fred]"
Loaded services file OK.
WARNING: passdb expand explicit = yes is deprecated
Server role: ROLE_STANDALONE
Press enter to see a dump of your service definitions

[global]
         workgroup = THOR
         server string = Samba Server
         log file = /var/log/smbd.%m
         max log size = 50
         dns proxy = No

[homes]
         comment = Home Directories
         read only = No
         browseable = No

[printers]
         comment = All Printers
         path = /var/spool/samba
         printable = Yes
         browseable = No

[fred]
         comment = Fred's Samba Tests
         path = /home/fred
         valid users = fred
         read only = No
thor:fred /home/fred> dmesg
OpenBSD 3.9-current (RAMDISK_CD) #1055: Tue Mar 28 15:18:06 MST 2006
     [hidden email]:/usr/src/sys/arch/i386/compile/RAMDISK_CD
cpu0: Intel(R) Pentium(R) III Mobile CPU 750MHz ("GenuineIntel"
686-class) 747 MHz
cpu0:
FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,MMX,FXSR,SSE
real mem  = 250572800 (244700K)
avail mem = 222818304 (217596K)
using 3084 buffers containing 12632064 bytes (12336K) of memory
mainbus0 (root)
bios0 at mainbus0: AT/286+(63) BIOS, date 07/05/02, BIOS32 rev. 0 @ 0xf9c65
apm0 at bios0: Power Management spec V1.2
apm0: flags 20102 dobusy 0 doidle 1
pcibios0 at bios0: rev 2.1 @ 0xf0000/0x10000
pcibios0: PCI IRQ Routing Table rev 1.0 @ 0xf0200/144 (7 entries)
pcibios0: PCI Interrupt Router at 000:07:0 ("Acer Labs M1533 ISA" rev 0x00)
pcibios0: PCI bus #2 is the last bus
bios0: ROM list: 0xc0000/0xc000 0xcc000/0x4000 0xe0000/0x10000!
cpu0 at mainbus0
pci0 at mainbus0 bus 0: configuration mode 1 (no bios)
pchb0 at pci0 dev 0 function 0 "Acer Labs M1644 PCI" rev 0x01
ppb0 at pci0 dev 1 function 0 "Acer Labs M5247 AGP/PCI-PC" rev 0x00
pci1 at ppb0 bus 1
vga1 at pci1 dev 0 function 0 "Trident CyberBlade XP/Ai1" rev 0x82
wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation)
pciide0 at pci0 dev 4 function 0 "Acer Labs M5229 UDMA IDE" rev 0xc3:
DMA, channel 0 wired to compatibility, channel 1 wired to compatibility
wd0 at pciide0 channel 0 drive 0: <TOSHIBA MK2003GAH>
wd0: 16-sector PIO, LBA, 19073MB, 39062520 sectors
wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 4
pciide0: channel 1 ignored (disabled)
"Acer Labs M5451 Audio" rev 0x01 at pci0 dev 6 function 0 not configured
pcib0 at pci0 dev 7 function 0 "Acer Labs M1533 ISA" rev 0x00
"Acer Labs M7101 Power" rev 0x00 at pci0 dev 8 function 0 not configured
fxp0 at pci0 dev 10 function 0 "Intel 8255x" rev 0x0d, i82550: irq 11,
address 00:00:39:7e:2d:91
inphy0 at fxp0 phy 1: i82555 10/100 PHY, rev. 4
ohci0 at pci0 dev 12 function 0 "NEC USB" rev 0x41: irq 11, version 1.0
usb0 at ohci0: USB revision 1.0
uhub0 at usb0
uhub0: NEC OHCI root hub, rev 1.00/1.00, addr 1
uhub0: 3 ports with 3 removable, self powered
ohci1 at pci0 dev 12 function 1 "NEC USB" rev 0x41: irq 11, version 1.0
usb1 at ohci1: USB revision 1.0
uhub1 at usb1
uhub1: NEC OHCI root hub, rev 1.00/1.00, addr 1
uhub1: 2 ports with 2 removable, self powered
"Intel PRO/Wireless 2100" rev 0x04 at pci0 dev 16 function 0 not configured
cbb0 at pci0 dev 17 function 0 "Toshiba ToPIC100 CardBus" rev 0x32: irq 11
"Toshiba SD Controller" rev 0x03 at pci0 dev 18 function 0 not configured
isa0 at pcib0
isadma0 at isa0
pckbc0 at isa0 port 0x60/5
pckbd0 at pckbc0 (kbd slot)
pckbc0: using irq 1 for kbd slot
wskbd0 at pckbd0: console keyboard, using wsdisplay0
npx0 at isa0 port 0xf0/16: using exception 16
cardslot0 at cbb0 slot 0 flags 0
cardbus0 at cardslot0: bus 2 device 0 cacheline 0x0, lattimer 0x0
pcmcia0 at cardslot0
biomask fffd netmask fffd ttymask ffff
rd0: fixed, 3800 blocks
dkcsum: wd0 matches BIOS drive 0x80
root on rd0a
rootdev=0x1100 rrootdev=0x2f00 rawdev=0x2f02
syncing disks... done

The operating system has halted.
Please press any key to reboot.

rebooting...
OpenBSD 3.9-current (GENERIC) #662: Tue Mar 28 14:48:09 MST 2006
     [hidden email]:/usr/src/sys/arch/i386/compile/GENERIC
cpu0: Intel(R) Pentium(R) III Mobile CPU 750MHz ("GenuineIntel"
686-class) 747 MHz
cpu0:
FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,MMX,FXSR,SSE
real mem  = 250572800 (244700K)
avail mem = 221650944 (216456K)
using 3084 buffers containing 12632064 bytes (12336K) of memory
mainbus0 (root)
bios0 at mainbus0: AT/286+(63) BIOS, date 07/05/02, BIOS32 rev. 0 @ 0xf9c65
apm0 at bios0: Power Management spec V1.2
apm0: battery life expectancy 100%
apm0: AC on, battery charge high
apm0: flags 20102 dobusy 0 doidle 1
pcibios0 at bios0: rev 2.1 @ 0xf0000/0x10000
pcibios0: PCI IRQ Routing Table rev 1.0 @ 0xf0200/144 (7 entries)
pcibios0: PCI Interrupt Router at 000:07:0 ("Acer Labs M1533 ISA" rev 0x00)
pcibios0: PCI bus #2 is the last bus
bios0: ROM list: 0xc0000/0xc000 0xe0000/0x10000!
cpu0 at mainbus0
pci0 at mainbus0 bus 0: configuration mode 1 (no bios)
pchb0 at pci0 dev 0 function 0 "Acer Labs M1644 PCI" rev 0x01
ppb0 at pci0 dev 1 function 0 "Acer Labs M5247 AGP/PCI-PC" rev 0x00
pci1 at ppb0 bus 1
vga1 at pci1 dev 0 function 0 "Trident CyberBlade XP/Ai1" rev 0x82
wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation)
wsdisplay0: screen 1-5 added (80x25, vt100 emulation)
pciide0 at pci0 dev 4 function 0 "Acer Labs M5229 UDMA IDE" rev 0xc3:
DMA, channel 0 wired to compatibility, channel 1 wired to compatibility
wd0 at pciide0 channel 0 drive 0: <TOSHIBA MK2003GAH>
wd0: 16-sector PIO, LBA, 19073MB, 39062520 sectors
wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 4
pciide0: channel 1 ignored (disabled)
autri0 at pci0 dev 6 function 0 "Acer Labs M5451 Audio" rev 0x01: irq 11
ac97: codec id 0x414b4d02 (Asahi Kasei AK4543)
ac97: codec features headphone, 18 bit DAC, 18 bit ADC, AKM 3D
audio0 at autri0
midi0 at autri0: <4DWAVE MIDI UART>
pcib0 at pci0 dev 7 function 0 "Acer Labs M1533 ISA" rev 0x00
alipm0 at pci0 dev 8 function 0 "Acer Labs M7101 Power" rev 0x00: 74KHz
clock
iic0 at alipm0
admtemp0 at iic0 addr 0x4c: adm1032
fxp0 at pci0 dev 10 function 0 "Intel 8255x" rev 0x0d, i82550: irq 11,
address 00:00:39:7e:2d:91
inphy0 at fxp0 phy 1: i82555 10/100 PHY, rev. 4
ohci0 at pci0 dev 12 function 0 "NEC USB" rev 0x41: irq 11, version 1.0
usb0 at ohci0: USB revision 1.0
uhub0 at usb0
uhub0: NEC OHCI root hub, rev 1.00/1.00, addr 1
uhub0: 3 ports with 3 removable, self powered
ohci1 at pci0 dev 12 function 1 "NEC USB" rev 0x41: irq 11, version 1.0
usb1 at ohci1: USB revision 1.0
uhub1 at usb1
uhub1: NEC OHCI root hub, rev 1.00/1.00, addr 1
uhub1: 2 ports with 2 removable, self powered
ipw0 at pci0 dev 16 function 0 "Intel PRO/Wireless 2100" rev 0x04: irq
11, address 00:04:23:57:a3:8c
cbb0 at pci0 dev 17 function 0 "Toshiba ToPIC100 CardBus" rev 0x32: irq 11
"Toshiba SD Controller" rev 0x03 at pci0 dev 18 function 0 not configured
isa0 at pcib0
isadma0 at isa0
pckbc0 at isa0 port 0x60/5
pckbd0 at pckbc0 (kbd slot)
pckbc0: using irq 1 for kbd slot
wskbd0 at pckbd0: console keyboard, using wsdisplay0
pms0 at pckbc0 (aux slot)
pckbc0: using irq 12 for aux slot
wsmouse0 at pms0 mux 0
pcppi0 at isa0 port 0x61
midi1 at pcppi0: <PC speaker>
spkr0 at pcppi0
npx0 at isa0 port 0xf0/16: using exception 16
cardslot0 at cbb0 slot 0 flags 0
cardbus0 at cardslot0: bus 2 device 0 cacheline 0x0, lattimer 0x0
pcmcia0 at cardslot0
biomask effd netmask effd ttymask ffff
pctr: 686-class user-level performance counters enabled
mtrr: Pentium Pro MTRR support
dkcsum: wd0 matches BIOS drive 0x80
root on wd0a
rootdev=0x0 rrootdev=0x300 rawdev=0x302
ugen0 at uhub0 port 1
ugen0: ACS ACR38 USB Reader, rev 1.10/1.00, addr 2j

screen scrape of top during a major file transfer:

load averages:  0.21,  0.17,  0.17
                               23:58:18
24 processes:  23 idle, 1 on processor
CPU states:  0.0% user,  0.0% nice,  1.4% system,  3.6% interrupt, 95.0%
idle
Memory: Real: 10M/36M act/tot  Free: 194M  Swap: 0K/512M used/tot

   PID USERNAME PRI NICE  SIZE   RES STATE    WAIT     TIME    CPU COMMAND
18428 fred       2    0 2488K 2620K sleep    netio    0:26  0.63% smbd
18415 fred       2    0 3248K 1436K sleep    select   0:00  0.00% sshd
19428 root       2    0  628K 1152K idle     select   0:00  0.00% sshd
15247 fred      28    0  460K 1052K onproc   -        0:00  0.00% top
  8238 root       2    0 1168K 1132K sleep    select   0:00  0.00% sendmail
26703 root       2    0 3232K 2088K idle     netio    0:00  0.00% sshd
  4675 fred      18    0  632K  496K idle     pause    0:00  0.00% ksh

Reply | Threaded
Open this post in threaded view
|

Re: diff: plug telldir/seekdir leaks and more

Josh-24
In reply to this post by Otto Moerbeek
Hey otto...

Ive not tested this particular patch, but the previous one, that Paul wrote, worked well
for me; no more memory leaks.

Im a little bit swamped with work right now, so I cant really test the new patch.

Cheers,
        Josh.


Otto Moerbeek wrote:

>On Fri, 24 Mar 2006, Otto Moerbeek wrote:
>
>  
>
>>Hi,
>>
>>This is a revised version of the diff Paul Thorn
>><[hidden email]> send some time ago. It's a mix of
>>Paul's diff, FreeBSD code and my own:
>>
>>- plug a huge leak that occurs if telldir() is called, but no
>>corresponding seekdir(). Samba is suffering from that.
>>- Use a data structure local to DIR to store the telldir data. FreeBSD
>>uses a linked list, I chose to use an array, which avoids allocating
>>lots of small chunks, the index becomes implicit as well, so we can
>>drop a field from the struct.
>>- Make sure that loc = telldir(); .... seekdir(loc); telldir() returns loc,
>>as POSIX requires. This is an area that can be improved, since it now
>>scans the array.
>>
>>Note that one documented behaviour is changed. POSIX does not require
>>it, and most other Unix-like system do not give that guarantee.
>>
>>Please review and test, especially on setup that uses telldir and
>>seekdir. AFAIK, no program in base does that, but at least samba
>>does.
>>    
>>
>
>Not a single reply to this so far. There must be some samba users
>hiding here.
>
>The diff is in snaps as well, to accomodate even easier testing.
>
>I'll crosspost this to misc@ as well, I know it's "not done", but I
>really need test reports.
>
> -Otto

Reply | Threaded
Open this post in threaded view
|

Re: diff: plug telldir/seekdir leaks and more

STeve Andre'
In reply to this post by Otto Moerbeek
On Friday 24 March 2006 04:15, Otto Moerbeek wrote:

> Hi,
>
> This is a revised version of the diff Paul Thorn
> <[hidden email]> send some time ago. It's a mix of
> Paul's diff, FreeBSD code and my own:
>
> - plug a huge leak that occurs if telldir() is called, but no
> corresponding seekdir(). Samba is suffering from that.
> - Use a data structure local to DIR to store the telldir data. FreeBSD
> uses a linked list, I chose to use an array, which avoids allocating
> lots of small chunks, the index becomes implicit as well, so we can
> drop a field from the struct.
> - Make sure that loc = telldir(); .... seekdir(loc); telldir() returns loc,
> as POSIX requires. This is an area that can be improved, since it now
> scans the array.
>
> Note that one documented behaviour is changed. POSIX does not require
> it, and most other Unix-like system do not give that guarantee.
>
> Please review and test, especially on setup that uses telldir and
> seekdir. AFAIK, no program in base does that, but at least samba
> does.
>
> -Otto

From the little testing I have done, this works.  However, my users have
never had problems in the 2+ years I've had an OpenBSD samba box.
But this hasn't introduced any problems.  I plan on testing it much more
this weekend/next week.

--STeve Andre'

Reply | Threaded
Open this post in threaded view
|

Re: diff: plug telldir/seekdir leaks and more

Otto Moerbeek
In reply to this post by Otto Moerbeek
Hi,

I just committed the diff. Thanks to everybody for testing.

There remains one issue: in some cases, the current diff is slow,
because it scans the list of previous telldir positions to avoid
allocating a new entry and to implement the requirement that
seekdir(loc); telldir() should return loc.

The diff below (against current) should solve that. It makes use of
the fact that telldirs alwas return a pos >= the current pos. So we
can remember the pos last set by seekdir() or returned by telldir() and
largely avoid having to scan the list all the time.

        -Otto

Index: opendir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/opendir.c,v
retrieving revision 1.16
diff -u -p -r1.16 opendir.c
--- opendir.c 1 Apr 2006 18:06:59 -0000 1.16
+++ opendir.c 1 Apr 2006 18:22:00 -0000
@@ -78,7 +78,7 @@ __opendir2(const char *name, int flags)
  dirp->dd_td->td_locs = NULL;
  dirp->dd_td->td_sz = 0;
  dirp->dd_td->td_loccnt = 0;
-
+ dirp->dd_td->td_last = 0;
 
  /*
  * If the machine's page size is an exact multiple of DIRBLKSIZ,
Index: telldir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/telldir.c,v
retrieving revision 1.8
diff -u -p -r1.8 telldir.c
--- telldir.c 1 Apr 2006 18:06:59 -0000 1.8
+++ telldir.c 1 Apr 2006 18:22:00 -0000
@@ -42,14 +42,18 @@
 long
 telldir(DIR *dirp)
 {
- long i = 0;
- struct ddloc *lp = dirp->dd_td->td_locs;
+ long i = dirp->dd_td->td_last;
+ struct ddloc *lp;
+
+ lp = &dirp->dd_td->td_locs[i];
 
  /* return previous telldir, if there */
  for (; i < dirp->dd_td->td_loccnt; i++, lp++) {
  if (lp->loc_seek == dirp->dd_seek &&
-    lp->loc_loc == dirp->dd_loc)
+    lp->loc_loc == dirp->dd_loc) {
+ dirp->dd_td->td_last = i;
  return (i);
+ }
  }
 
  if (dirp->dd_td->td_loccnt == dirp->dd_td->td_sz) {
@@ -65,6 +69,7 @@ telldir(DIR *dirp)
  dirp->dd_td->td_loccnt++;
  lp->loc_seek = dirp->dd_seek;
  lp->loc_loc = dirp->dd_loc;
+ dirp->dd_td->td_last = i;
  return (i);
 }
 
@@ -81,6 +86,7 @@ __seekdir(DIR *dirp, long loc)
  if (loc < 0 || loc >= dirp->dd_td->td_loccnt)
  return;
  lp = &dirp->dd_td->td_locs[loc];
+ dirp->dd_td->td_last = loc;
  if (lp->loc_loc == dirp->dd_loc && lp->loc_seek == dirp->dd_seek)
  return;
  (void) lseek(dirp->dd_fd, (off_t)lp->loc_seek, SEEK_SET);
Index: telldir.h
===================================================================
RCS file: /cvs/src/lib/libc/gen/telldir.h,v
retrieving revision 1.1
diff -u -p -r1.1 telldir.h
--- telldir.h 1 Apr 2006 18:06:59 -0000 1.1
+++ telldir.h 1 Apr 2006 18:22:00 -0000
@@ -55,6 +55,7 @@ struct _telldir {
  struct ddloc *td_locs; /* locations */
  size_t td_sz; /* size of locations */
  long td_loccnt; /* index of entry for sequential readdir's */
+ long td_last; /* last tell/seekdir */
 };
 
 void __seekdir(DIR *, long);