On Jan 4, 2012, at 3:08 PM, Jim Meyering wrote:
> Chuck Lever wrote:
>> To support FedFS and NFS junctions without introducing additional
>> build-time or run-time dependencies on nfs-utils, the community has
>> chosen to use a dynamically loadable library to handle junction
>> resolution.
> ...
>
> Hi Chuck,
>
> You may just be following existing practice in nfs-utils (there
> are numerous other examples of this off-by-one error[1]), but the
> post-snprintf length check must fail when the returned length is
> equal to the specified buffer length.
I can fix and repost if there are more comments on this patch. Otherwise, once Steve commits it you might consider fixing all of these at once.
>
>> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
>> index d2ae456..d4f04ab 100644
>> --- a/utils/mountd/cache.c
>> +++ b/utils/mountd/cache.c
>> @@ -802,6 +802,229 @@ lookup_export(char *dom, char *path, struct addrinfo *ai)
>> return found;
>> }
>>
>> +#ifdef HAVE_NFS_PLUGIN_H
>> +#include <dlfcn.h>
>> +#include <nfs-plugin.h>
>> +
>> +/*
>> + * Walk through a set of FS locations and build a set of export options.
>> + * Returns true if all went to plan; otherwise, false.
>> + */
>> +static _Bool
>> +locations_to_options(struct jp_ops *ops, nfs_fsloc_set_t locations,
>> + char *options, size_t remaining, int *ttl)
>> +{
>> + char *server, *last_path, *rootpath, *ptr;
>> + _Bool seen = false;
>> +
>> + last_path = NULL;
>> + rootpath = NULL;
>> + server = NULL;
>> + ptr = options;
>> + *ttl = 0;
>> +
>> + for (;;) {
>> + enum jp_status status;
>> + int len;
>> +
>> + status = ops->jp_get_next_location(locations, &server,
>> + &rootpath, ttl);
>> + if (status == JP_EMPTY)
>> + break;
>> + if (status != JP_OK) {
>> + xlog(D_GENERAL, "%s: failed to parse location: %s",
>> + __func__, ops->jp_error(status));
>> + goto out_false;
>> + }
>> + xlog(D_GENERAL, "%s: Location: %s:%s",
>> + __func__, server, rootpath);
>> +
>> + if (last_path && strcmp(rootpath, last_path) == 0) {
>> + len = snprintf(ptr, remaining, "+%s", server);
>> + if (len < 0) {
>> + xlog(D_GENERAL, "%s: snprintf: %m", __func__);
>> + goto out_false;
>> + }
>> + if ((size_t)len > remaining) {
>
> s/>/>=/
>
> snprintf returning the specified size indicates that the
> result+NUL did not fit in the specified buffer.
>
>> + xlog(D_GENERAL, "%s: options buffer overflow", __func__);
>> + goto out_false;
>> + }
>> + remaining -= (size_t)len;
>> + ptr += len;
>> + } else {
>> + if (last_path == NULL)
>> + len = snprintf(ptr, remaining, "refer=%s@%s",
>> + rootpath, server);
>> + else
>> + len = snprintf(ptr, remaining, ":%s@%s",
>> + rootpath, server);
>> + if (len < 0) {
>> + xlog(D_GENERAL, "%s: snprintf: %m", __func__);
>> + goto out_false;
>> + }
>> + if ((size_t)len > remaining) {
>
> Same here.
>
>> + xlog(D_GENERAL, "%s: options buffer overflow",
>> + __func__);
>> + goto out_false;
>> + }
>> + remaining -= (size_t)len;
>> + ptr += len;
>> + last_path = rootpath;
>> + }
>
> [1] Here are other examples of that error:
>
> $ git grep -A13 snprintf|grep ' > '
> support/nfs/cacheio.c- if (len > *lp)
> support/nfs/cacheio.c- if (len > *lp)
> support/nfs/getport.c- if (len < 0 || (size_t)len > count)
> utils/exportfs/exportfs.c- if (fname_len > PATH_MAX) {
> utils/gssd/gssd_proc.c- if (nbytes > INFOBUFLEN)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
