From: Daniel Ottavio Date: Sat, 9 Apr 2005 18:15:35 +0000 (+0000) Subject: * amd/nfs_subr.c (mp_to_fh): Replace xstrlcpy with memcpy because the X-Git-Tag: am-utils-6_1_rc2~14 X-Git-Url: https://git.fsl.cs.stonybrook.edu/?a=commitdiff_plain;h=e1e1180dfd2ece3b6a6eb55bb87d5ef5a4b4e9f0;p=am-utils-6.0.git * 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. --- diff --git a/ChangeLog b/ChangeLog index 36a15f3..e65784b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,28 @@ +2005-04-09 Daniel P. Ottavio + + * 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 * include/am_utils.h (XFREE): XFREE() should nullify the pointer diff --git a/NEWS b/NEWS index a2ad885..5cd20be 100644 --- 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 diff --git a/amd/nfs_subr.c b/amd/nfs_subr.c index fd3b347..6fda0e9 100644 --- a/amd/nfs_subr.c +++ b/amd/nfs_subr.c @@ -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; diff --git a/amd/opts.c b/amd/opts.c index 81b065e..38f04ff 100644 --- a/amd/opts.c +++ b/amd/opts.c @@ -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 */ diff --git a/libamu/mount_fs.c b/libamu/mount_fs.c index 21f3851..ccb0947 100644 --- a/libamu/mount_fs.c +++ b/libamu/mount_fs.c @@ -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; diff --git a/libamu/strutil.c b/libamu/strutil.c index 4dfb928..a3d43ef 100644 --- a/libamu/strutil.c +++ b/libamu/strutil.c @@ -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); }