Re: [PATCH 04/15] mkfs: validate all input values

From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 04/15] mkfs: validate all input values
Date: Mon, 2 Dec 2013 09:04:20 -0800	[thread overview]
Message-ID: <20131202170420.GA14935@infradead.org> (raw)
In-Reply-To: <1385689430-10103-5-git-send-email-david@fromorbit.com>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 2637 bytes --]

On Fri, Nov 29, 2013 at 12:43:39PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Right now, mkfs does a poor job of input validation of values. Many
> parameters do not check for trailing garbage and so will pass
> obviously invalid values as OK. Some don't even detect completely
> invalid values, leaving it for other checks later on to fail due to
> a bad value conversion - these tend to rely on atoi() implicitly
> returning a sane value when it is passed garbage, and atoi gives no
> guarantee of the return value when passed garbage.

Would be useful to have a test case for some of these garbage values..

> Finally, the block size of the filesystem is not known until all
> the options have been parsed and we can determine if the default is
> to be used. This means any parameter that relies on using conversion
> from filesystem block size (the "NNNb" format) requires the block
> size to first be specified on the command line so it is known.
> 
> Similarly, we make the same rule for specifying counts in sectors.
> This is a change from the existing behaviour that assumes sectors
> are 512 bytes unless otherwise changed on the command line. This,
> unfortunately, leads to complete silliness where you can specify the
> sector size as a count of sectors. It also means that you can do
> some conversions with 512 byte sector sizes, and others with
> whatever was specified on the command line, meaning the mkfs
> behaviour changes depending in where in the command line the sector
> size is changed....

I wonder if this might break some existing uses.  The whole notion of
512byte sectors is so ingrained in most people that this doesn't sound
as stupid as it is.

Maybe just warn about that particular case for now instead of outright
rejecting it?

> +	creds.cr_uid = getnum(getstr(pp), 0, 0, false);
> +	creds.cr_gid = getnum(getstr(pp), 0, 0, false);

Not that I really care deeply, but requiring uids to be numeric seems a
little silly.  Maybe we should put accepting user and groups names on a
beginners todo list somewhere.

> +long long
> +getnum(
> +	const char	*str,
> +	unsigned int	blocksize,
> +	unsigned int	sectorsize,
> +	bool		convert)
> +{
> +	long long	i;
> +	char		*sp;
> +
> +	if (convert)
> +		return cvtnum(blocksize, sectorsize, str);
> +
> +	i = strtoll(str, &sp, 0);
> +	if (i == 0 && sp == str)
> +		return -1LL;
> +	if (*sp != '\0')
> +		return -1LL; /* trailing garbage */
> +	return i;
> +}

So this function does two totally different things based on the last
parameter?  Unless the answers is one of the next patches will fix it
¿ thyink it should be split.

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs