* amd/nfs_subr.c (mp_to_fh): Replace xstrlcpy with memcpy because the
authorDaniel Ottavio <ottavio@fsl.cs.sunysb.edu>
Sat, 9 Apr 2005 18:15:35 +0000 (18:15 +0000)
committerDaniel Ottavio <ottavio@fsl.cs.sunysb.edu>
Sat, 9 Apr 2005 18:15:35 +0000 (18:15 +0000)
source buffer is treated more as a filehandle than a string.

* amd/nfs_subr.c (fh_to_mp3): Replace xstrlcpy with memcpy because the
source buffer is treated more as a filehandle than a string.

* amd/opts.c (free_op): No longer need to assign pointer to NULL
after XFREE.

* amd/opts.c (expand_op): Revert back to using strncpy() instead
of xstrlcpy.  The code is correct and relies on the semantics of
strncpy.

* libamu/mount_fs.c (compute_nfs_args): Leave XXX warning that use
of xstrlcpy in NFS_HN_DREF may corrupt a struct nfs_args, or
truncate our concocted "hostname:/path" string prematurely if the
nap->hostname field is ever less than 64 bytes long
(MAXHOSTNAMELEN).

* libamu/util.c (xstrlcpy): Return immediately if len is 0 to
avoid unnecessary work.  Log an error and return if len is less
than 0.

ChangeLog
NEWS
amd/nfs_subr.c
amd/opts.c
libamu/mount_fs.c
libamu/strutil.c

index 36a15f309f1a8f4d7819c5c6d180e91d9c397515..e65784b5a855180fe5d1a25c8dae96c768fc5b52 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,28 @@
+2005-04-09  Daniel P. Ottavio  <dottavio@ic.sunysb.edu>
+
+       * amd/nfs_subr.c (mp_to_fh): Replace xstrlcpy with memcpy because the
+       source buffer is treated more as a filehandle than a string.
+
+       * amd/nfs_subr.c (fh_to_mp3): Replace xstrlcpy with memcpy because the
+       source buffer is treated more as a filehandle than a string.
+
+       * amd/opts.c (free_op): No longer need to assign pointer to NULL
+       after XFREE.
+
+       * amd/opts.c (expand_op): Revert back to using strncpy() instead
+       of xstrlcpy.  The code is correct and relies on the semantics of
+       strncpy.
+
+       * libamu/mount_fs.c (compute_nfs_args): Leave XXX warning that use
+       of xstrlcpy in NFS_HN_DREF may corrupt a struct nfs_args, or
+       truncate our concocted "hostname:/path" string prematurely if the
+       nap->hostname field is ever less than 64 bytes long
+       (MAXHOSTNAMELEN).
+
+       * libamu/util.c (xstrlcpy): Return immediately if len is 0 to
+       avoid unnecessary work.  Log an error and return if len is less
+       than 0.
+       
 2005-04-07  Erez Zadok  <ezk@cs.sunysb.edu>
 
        * include/am_utils.h (XFREE): XFREE() should nullify the pointer
diff --git a/NEWS b/NEWS
index a2ad88582efa7d185980b24ce90e9032ac48508e..5cd20be00068b7610803d7ee931b36483a69045b 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -1,7 +1,8 @@
 *** Notes specific to am-utils version 6.1-rc2
 
-Using a custom version of strlcpy instead of strncpy, to minimize string
-overlow changes.  Code is safer now.
+Using a custom version of strlcpy instead of strncpy (but only where
+it makes sense), to minimize string overlow changes.  Audited all use
+of strncpy/strlcpy to ensure safety.
 
 - bugs fixed:
        * pawd handles all file systems
index fd3b347954d2714b7ac766800fe126303a286641..6fda0e98543d27469c85aef484abaae1cfdc3c84 100644 (file)
@@ -37,7 +37,7 @@
  * SUCH DAMAGE.
  *
  *
- * $Id: nfs_subr.c,v 1.27 2005/04/07 05:50:38 ezk Exp $
+ * $Id: nfs_subr.c,v 1.28 2005/04/09 18:15:35 ottavio Exp $
  *
  */
 
@@ -614,8 +614,14 @@ fh_to_mp3(am_nfs_fh *fhp, int *rp, int vop)
 
   if (fp->fhh_type != 0) {
     /* New filehandle type */
-    char *path = xmalloc(sizeof(*fhp));
-    xstrlcpy(path, (char *) fhp, sizeof(*fhp));
+    int len = sizeof(*fhp);
+    char *path = xmalloc(len+1);
+    /*
+     * Because fhp is treated as a filehandle we use memcpy
+     * instead of xstrlcpy.
+     */
+    memcpy(path, (char *) fhp, len);
+    path[len] = '\0';
     /* dlog("fh_to_mp3: new filehandle: %s", path); */
 
     ap = path_to_exported_ap(path);
@@ -762,7 +768,11 @@ mp_to_fh(am_node *mp, am_nfs_fh *fhp)
   pathlen = strlen(mp->am_path);
   if (pathlen <= sizeof(*fhp)) {
     /* dlog("mp_to_fh: new filehandle: %s", mp->am_path); */
-    xstrlcpy((char *) fhp, mp->am_path, pathlen);
+    /*
+     * Because fhp is treated as a filehandle we use memcpy instead of
+     * xstrlcpy.
+     */
+    memcpy((char *) fhp, mp->am_path, pathlen); /* making a filehandle */
   } else {
     struct am_fh *fp = (struct am_fh *) fhp;
 
index 81b065ea40feb7671169a8c26b1bb16d38aa8ab7..38f04ff2b2fbf4b6d7530ebfe4b809501e758350 100644 (file)
@@ -37,7 +37,7 @@
  * SUCH DAMAGE.
  *
  *
- * $Id: opts.c,v 1.37 2005/04/07 05:50:38 ezk Exp $
+ * $Id: opts.c,v 1.38 2005/04/09 18:15:35 ottavio Exp $
  *
  */
 
@@ -952,7 +952,6 @@ free_op(opt_apply *p, int b)
 {
   if (*p->opt) {
     XFREE(*p->opt);
-    *p->opt = 0;
   }
 }
 
@@ -1026,13 +1025,19 @@ expand_op(char *opt, int sel_p)
      */
     {
       int len = dp - cp;
-
-      if (BUFSPACE(ep, len)) {
-       xstrlcpy(ep, cp, len);
-       ep += len;
-      } else {
-       plog(XLOG_ERROR, EXPAND_ERROR, opt);
-       goto out;
+      
+      if (len > 0) {
+       if (BUFSPACE(ep, len)) { 
+         /* 
+          * We use strncpy (not xstrlen) because 'ep' relies on it's symantics.
+          * BUFSPACE guarantees that ep can hold len.
+          */
+         strncpy(ep, cp, len);
+         ep += len;
+       } else {
+         plog(XLOG_ERROR, EXPAND_ERROR, opt);
+         goto out;
+       }
       }
     }
 
@@ -1115,9 +1120,15 @@ expand_op(char *opt, int sel_p)
       /*
        * Put the string into another buffer so
        * we can do comparisons.
+       *
+       * We use strncpy here (not xstrlcpy) because the dest is meant
+       * to be truncated and we don't want to log it as an error.  The
+       * use of the BUFSPACE macro above guarantees the safe use of
+       * strncpy with nbuf.
        */
-      xstrlcpy(nbuf, cp, len);
-
+      strncpy(nbuf, cp, len);
+      nbuf[len] = '\0';
+      
       /*
        * Advance cp
        */
index 21f3851bc02842b0d54eda89dff9102264b25ce1..ccb0947b1e2e46782bd1f14494af72447d54904e 100644 (file)
@@ -37,7 +37,7 @@
  * SUCH DAMAGE.
  *
  *
- * $Id: mount_fs.c,v 1.49 2005/03/21 00:16:53 ezk Exp $
+ * $Id: mount_fs.c,v 1.50 2005/04/09 18:15:35 ottavio Exp $
  *
  */
 
@@ -491,6 +491,11 @@ compute_nfs_args(nfs_args_t *nap,
   /************************************************************************/
   /*** HOST NAME                                                       ***/
   /************************************************************************/
+  /*
+   * XXX: warning, using xstrlcpy in NFS_HN_DREF, which may corrupt a
+   * struct nfs_args, or truncate our concocted "hostname:/path"
+   * string prematurely.
+   */
   NFS_HN_DREF(nap->hostname, host_name);
 #ifdef MNT2_NFS_OPT_HOSTNAME
   nap->flags |= MNT2_NFS_OPT_HOSTNAME;
index 4dfb9288a1ed9abb595f99120a25c3039020fd26..a3d43ef6f01fc942c91dbfdda43cfcfc054ecea4 100644 (file)
@@ -37,7 +37,7 @@
  * SUCH DAMAGE.
  *
  *
- * $Id: strutil.c,v 1.15 2005/04/07 05:09:16 ezk Exp $
+ * $Id: strutil.c,v 1.16 2005/04/09 18:15:35 ottavio Exp $
  *
  */
 
@@ -93,6 +93,12 @@ str3cat(char *p, char *s1, char *s2, char *s3)
 void
 xstrlcpy(char *dst, const char *src, size_t len)
 {
+  if (len < 0) {
+    plog(XLOG_ERROR, "xstrlcpy: illegal len %d", len);
+    return;
+  }
+  if (len == 0)
+    return;
   if (strlcpy(dst, src, len) >= len)
     plog(XLOG_ERROR, "xstrlcpy: string \"%s\" truncated to \"%s\"", src, dst);
 }