On Fri, Jun 19, 2015 at 01:01:55PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Testing logarithmic paramters like "-n log=<num>" shows that we do a
> terrible job of validating such input. e.g.:
>
> # mkfs.xfs -f -n log=456858480 /dev/vda
> .....
> naming =version 2 bsize=65536 ascii-ci=0 ftype=0
> ....
>
> Yeah, I just asked for a block size of 2^456858480, and it didn't
> get rejected. Great, isn't it?
>
> So, factor out the parsing of logarithmic parameters, and pass in
> the maximum valid value that they can take. These maximum values
> might not be completely accurate (e.g. block/sector sizes will
> affect the eventual valid maximum) but we can get rid of all the
> overflows and stupidities before we get to fine-grained validity
> checking later in mkfs once things like block and sector sizes have
> been finalised.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Jan Ťulák <jtulak@xxxxxxxxxx>
> ---
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> mkfs/xfs_mkfs.c | 79 +++++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 51 insertions(+), 28 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 6b9e991..6f0aa55 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1044,6 +1044,27 @@ getbool(
> return c ? true : false;
> }
>
> +static int
> +getnum_checked(
> + const char *str,
> + long long min_val,
> + long long max_val,
> + const char *illegal_str,
> + char reqval_char,
> + char *reqval_opts[],
> + int reqval_optind)
> +{
> + long long c;
> +
> + if (!str || *str == '\0')
> + reqval(reqval_char, reqval_opts, reqval_optind);
> +
> + c = getnum(str, 0, 0, false);
> + if (c < min_val || c > max_val)
> + illegal(str, illegal_str);
> + return c;
> +}
> +
> int
> main(
> int argc,
> @@ -1200,16 +1221,16 @@ main(
>
> switch (getsubopt(&p, (constpp)bopts, &value)) {
> case B_LOG:
> - if (!value || *value == '\0')
> - reqval('b', bopts, B_LOG);
> if (blflag)
> respec('b', bopts, B_LOG);
> if (bsflag)
> conflict('b', bopts, B_SIZE,
> B_LOG);
> - blocklog = getnum(value, 0, 0, false);
> - if (blocklog <= 0)
> - illegal(value, "b log");
> + blocklog = getnum_checked(value,
> + XFS_MIN_BLOCKSIZE_LOG,
> + XFS_MAX_BLOCKSIZE_LOG,
> + "b log", 'b', bopts,
> + B_LOG);
> blocksize = 1 << blocklog;
> blflag = 1;
> break;
> @@ -1346,16 +1367,16 @@ main(
> nodsflag = 1;
> break;
> case D_SECTLOG:
> - if (!value || *value == '\0')
> - reqval('d', dopts, D_SECTLOG);
> if (slflag)
> respec('d', dopts, D_SECTLOG);
> if (ssflag)
> conflict('d', dopts, D_SECTSIZE,
> D_SECTLOG);
> - sectorlog = getnum(value, 0, 0, false);
> - if (sectorlog <= 0)
> - illegal(value, "d sectlog");
> + sectorlog = getnum_checked(value,
> + XFS_MIN_SECTORSIZE_LOG,
> + XFS_MAX_SECTORSIZE_LOG,
> + "d sectlog", 'd', dopts,
> + D_SECTLOG);
> sectorsize = 1 << sectorlog;
> slflag = 1;
> break;
> @@ -1420,9 +1441,11 @@ main(
> if (isflag)
> conflict('i', iopts, I_SIZE,
> I_LOG);
> - inodelog = getnum(value, 0, 0, false);
> - if (inodelog <= 0)
> - illegal(value, "i log");
> + inodelog = getnum_checked(value,
> + XFS_DINODE_MIN_LOG,
> + XFS_DINODE_MAX_LOG,
> + "i log", 'i', iopts,
> + I_LOG);
> isize = 1 << inodelog;
> ilflag = 1;
> break;
> @@ -1591,16 +1614,16 @@ main(
> lsflag = 1;
> break;
> case L_SECTLOG:
> - if (!value || *value == '\0')
> - reqval('l', lopts, L_SECTLOG);
> if (lslflag)
> respec('l', lopts, L_SECTLOG);
> if (lssflag)
> conflict('l', lopts, L_SECTSIZE,
> L_SECTLOG);
> - lsectorlog = getnum(value, 0, 0, false);
> - if (lsectorlog <= 0)
> - illegal(value, "l sectlog");
> + lsectorlog = getnum_checked(value,
> + XFS_MIN_SECTORSIZE_LOG,
> + XFS_MAX_SECTORSIZE_LOG,
> + "l sectlog", 'l', lopts,
> + L_SECTLOG);
> lsectorsize = 1 << lsectorlog;
> lslflag = 1;
> break;
> @@ -1673,16 +1696,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>
> switch (getsubopt(&p, (constpp)nopts, &value)) {
> case N_LOG:
> - if (!value || *value == '\0')
> - reqval('n', nopts, N_LOG);
> if (nlflag)
> respec('n', nopts, N_LOG);
> if (nsflag)
> conflict('n', nopts, N_SIZE,
> N_LOG);
> - dirblocklog = getnum(value, 0, 0, false);
> - if (dirblocklog <= 0)
> - illegal(value, "n log");
> + dirblocklog = getnum_checked(value,
> + XFS_MIN_REC_DIRSIZE,
> + XFS_MAX_BLOCKSIZE_LOG,
> + "n log", 'n', nopts,
> + N_LOG);
> dirblocksize = 1 << dirblocklog;
> nlflag = 1;
> break;
> @@ -1801,16 +1824,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> switch (getsubopt(&p, (constpp)sopts, &value)) {
> case S_LOG:
> case S_SECTLOG:
> - if (!value || *value == '\0')
> - reqval('s', sopts, S_SECTLOG);
> if (slflag || lslflag)
> respec('s', sopts, S_SECTLOG);
> if (ssflag || lssflag)
> conflict('s', sopts, S_SECTSIZE,
> S_SECTLOG);
> - sectorlog = getnum(value, 0, 0, false);
> - if (sectorlog <= 0)
> - illegal(value, "s sectlog");
> + sectorlog = getnum_checked(value,
> + XFS_MIN_SECTORSIZE_LOG,
> + XFS_MAX_SECTORSIZE_LOG,
> + "s sectlog", 's', sopts,
> + S_SECTLOG);
> lsectorlog = sectorlog;
> sectorsize = 1 << sectorlog;
> lsectorsize = sectorsize;
> --
> 2.1.0
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
