On Fri, Mar 09, 2018 at 12:44:49PM -0600, Scott Cheloha wrote:
> Hey,
> 
> I think we can improve how we handle length and skip in both hexdump and
> od.  While we're at it we can improve our documentation a bit and bring
> od(1) further into alignment with POSIX.
> 
> skip is an off_t, so unless I'm misunderstanding sys/_types.h it's
> always 64-bit so we should always snag it with strtoll(3).  Right
> now we do that for hexdump but not for od.
> 
> I have done the strtol -> strtoll in od for the XSI offset thing, but
> I'm unsure how to proceed.  What should we do if the offset overflows?
> Error out?  Set skip to zero and return?
> 
> skip is used by both programs, so move its definition into hexdump.c.
> 
> For both programs, check that (a) multiplying skip by its suffix doesn't
> overflow and (b) that we don't have an invalid suffix or more than one
> character for the suffix.
> 
> Add finer-grained error messages when parsing both offset and length in
> both programs.  The variable name is "skip" but the documentation calls
> it "offset," so I'm using the latter in the error messages.
> 
> For od, POSIX.1-2008 (and earlier specs, too, it seems) says length can
> be octal or hexidecimal, so snag it with strtol, not atoi, and note this
> possibility in od.1.
> 
> Unless I'm misunderstanding sys/limits.h, bumping length from a long int to a
> long long int gives 32-bit platforms a larger range for length without needing
> to change any other code, so do that: snag it with strtoll in both programs.
> 
> In the documentation, I think the use of "otherwise" is awkward when
> introducing the possibility of an octal input because it signals a sort of
> default and we've already said that lengths are decimal by default.  It's
> cleaner to introduce octal inputs in a new sentence.
> 

the ", otherwise," part is indeed clumsy. it should be replaced entirely
with a semi-colon. i'm ok with starting a new sentence too, but
logically i'd prefer a semi-colon (the two sentences are strongly
related), plus it would be shorter. but whatever way, i'm ok with the
man changes.

jmc

> Thoughts?
> 
> --
> Scott Cheloha
> 
> Index: usr.bin/hexdump/hexdump.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/hexdump/hexdump.1,v
> retrieving revision 1.26
> diff -u -p -r1.26 hexdump.1
> --- usr.bin/hexdump/hexdump.1 12 Sep 2015 11:59:38 -0000      1.26
> +++ usr.bin/hexdump/hexdump.1 9 Mar 2018 18:36:52 -0000
> @@ -96,8 +96,8 @@ With a leading
>  or
>  .Cm 0X ,
>  .Ar length
> -is interpreted as a hexadecimal number,
> -otherwise, with a leading
> +is interpreted as a hexadecimal number.
> +With a leading
>  .Cm 0 ,
>  .Ar length
>  is interpreted as an octal number.
> @@ -118,8 +118,8 @@ With a leading
>  or
>  .Cm 0X ,
>  .Ar offset
> -is interpreted as a hexadecimal number,
> -otherwise, with a leading
> +is interpreted as a hexadecimal number.
> +With a leading
>  .Cm 0 ,
>  .Ar offset
>  is interpreted as an octal number.
> Index: usr.bin/hexdump/hexdump.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/hexdump/hexdump.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 hexdump.c
> --- usr.bin/hexdump/hexdump.c 8 Feb 2016 21:05:51 -0000       1.20
> +++ usr.bin/hexdump/hexdump.c 9 Mar 2018 18:36:52 -0000
> @@ -42,9 +42,10 @@
>  FS *fshead;                          /* head of format strings */
>  int blocksize;                               /* data block size */
>  int exitval;                         /* final exit value */
> -long length = -1;                    /* max bytes to read */
> +long long length = -1;                       /* max bytes to read */
>  char *iobuf;                         /* stdio I/O buffer */
>  size_t iobufsiz;                     /* size of stdio I/O buffer */
> +off_t skip;                          /* bytes to skip */
>  
>  int
>  main(int argc, char *argv[])
> Index: usr.bin/hexdump/hexdump.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/hexdump/hexdump.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 hexdump.h
> --- usr.bin/hexdump/hexdump.h 15 Mar 2016 04:19:13 -0000      1.12
> +++ usr.bin/hexdump/hexdump.h 9 Mar 2018 18:36:52 -0000
> @@ -76,7 +76,7 @@ extern int odmode;                  /* od compatibility
>  extern FU *endfu;                    /* format at end-of-data */
>  extern int exitval;                  /* final exit value */
>  extern FS *fshead;                   /* head of format strings list */
> -extern long length;                  /* max bytes to read */
> +extern long long length;             /* max bytes to read */
>  extern off_t skip;                   /* bytes to skip */
>  extern char *iobuf;                  /* stdio I/O buffer */
>  extern size_t iobufsiz;                      /* size of stdio I/O buffer */
> Index: usr.bin/hexdump/hexsyntax.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/hexdump/hexsyntax.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 hexsyntax.c
> --- usr.bin/hexdump/hexsyntax.c       15 Mar 2016 04:19:13 -0000      1.13
> +++ usr.bin/hexdump/hexsyntax.c       9 Mar 2018 18:36:52 -0000
> @@ -34,6 +34,7 @@
>  
>  #include <err.h>
>  #include <errno.h>
> +#include <limits.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -41,14 +42,13 @@
>  
>  #include "hexdump.h"
>  
> -off_t skip;                          /* bytes to skip */
> -
>  static __dead void    usage(void);
>  
>  void
>  newsyntax(int argc, char ***argvp)
>  {
> -     int ch;
> +     long long multiplier;
> +     int ch, unknown;
>       char *p, **argv;
>  
>       argv = *argvp;
> @@ -79,9 +79,13 @@ newsyntax(int argc, char ***argvp)
>                       break;
>               case 'n':
>                       errno = 0;
> -                     if ((length = strtol(optarg, NULL, 0)) < 0 ||
> -                         errno != 0)
> -                             errx(1, "%s: bad length value", optarg);
> +                     length = strtoll(optarg, &p, 0);
> +                     if (optarg[0] == '\0' || *p != '\0')
> +                             errx(1, "length is invalid: %s", optarg);
> +                     if (length < 0)
> +                             errx(1, "length is too small: %s", optarg);
> +                     if (length == LLONG_MAX && errno == ERANGE)
> +                             errx(1, "length is too large: %s", optarg);
>                       break;
>               case 'o':
>                       add("\"%07.7_Ax\n\"");
> @@ -89,20 +93,35 @@ newsyntax(int argc, char ***argvp)
>                       break;
>               case 's':
>                       errno = 0;
> -                     if ((skip = (off_t)strtoll(optarg, &p, 0)) < 0 ||
> -                         errno != 0)
> -                             errx(1, "%s: bad skip value", optarg);
> +                     skip = strtoll(optarg, &p, 0);
> +                     if (optarg[0] == '\0' || p == optarg)
> +                             errx(1, "offset is invalid 1: %s", optarg);
> +                     if (skip < 0)
> +                             errx(1, "offset is too small: %s", optarg);
> +                     if (skip == LLONG_MAX && errno == ERANGE)
> +                             errx(1, "offset is too large: %s", optarg);
> +                     unknown = 0;
>                       switch(*p) {
> +                     case '\0':
> +                             multiplier = 1;
> +                             break;
>                       case 'b':
> -                             skip *= 512;
> +                             multiplier = 512;
>                               break;
>                       case 'k':
> -                             skip *= 1024;
> +                             multiplier = 1024;
>                               break;
>                       case 'm':
> -                             skip *= 1048576;
> +                             multiplier = 1048576;
>                               break;
> +                     default:
> +                             unknown = 1;
>                       }
> +                     if (unknown || (p[0] != '\0' && p[1] != '\0'))
> +                             errx(1, "offset has invalid unit: %s", optarg);
> +                     if (skip > LLONG_MAX / multiplier)
> +                             errx(1, "offset is too large: %s", optarg);
> +                     skip *= multiplier;
>                       break;
>               case 'v':
>                       vflag = ALL;
> Index: usr.bin/hexdump/od.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/hexdump/od.1,v
> retrieving revision 1.31
> diff -u -p -r1.31 od.1
> --- usr.bin/hexdump/od.1      16 Sep 2015 08:47:26 -0000      1.31
> +++ usr.bin/hexdump/od.1      9 Mar 2018 18:36:52 -0000
> @@ -138,8 +138,8 @@ With a leading
>  or
>  .Cm 0X ,
>  .Ar offset
> -is interpreted as a hexadecimal number,
> -otherwise, with a leading
> +is interpreted as a hexadecimal number.
> +With a leading
>  .Cm 0 ,
>  .Ar offset
>  is interpreted as an octal number.
> @@ -181,6 +181,19 @@ Same as
>  Interpret only
>  .Ar length
>  bytes of input.
> +By default,
> +.Ar length
> +is interpreted as a decimal number.
> +With a leading
> +.Cm 0x
> +or
> +.Cm 0X ,
> +.Ar length
> +is interpreted as a hexadecimal number.
> +With a leading
> +.Cm 0 ,
> +.Ar length
> +is interpreted as an octal number.
>  .It Fl O
>  .Em Four-byte octal display .
>  Display the input offset in octal, followed by four
> Index: usr.bin/hexdump/odsyntax.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/hexdump/odsyntax.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 odsyntax.c
> --- usr.bin/hexdump/odsyntax.c        30 May 2017 05:58:44 -0000      1.28
> +++ usr.bin/hexdump/odsyntax.c        9 Mar 2018 18:36:52 -0000
> @@ -34,6 +34,8 @@
>  
>  #include <ctype.h>
>  #include <err.h>
> +#include <errno.h>
> +#include <limits.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> @@ -80,8 +82,9 @@ void
>  oldsyntax(int argc, char ***argvp)
>  {
>       static char empty[] = "", padding[] = PADDING;
> -     int ch;
>       char *p, **argv;
> +     long long multiplier;
> +     int ch, unknown;
>  
>  #define      TYPE_OFFSET     7
>       add("\"%07.7_Ao\n\"");
> @@ -150,23 +153,46 @@ oldsyntax(int argc, char ***argvp)
>                       odadd("8/2 \" %6d \" \"\\n\"");
>                       break;
>               case 'j':
> -                     if ((skip = strtol(optarg, &p, 0)) < 0)
> -                             errx(1, "%s: bad skip value", optarg);
> +                     errno = 0;
> +                     skip = strtoll(optarg, &p, 0);
> +                     if (optarg[0] == '\0' || p == optarg)
> +                             errx(1, "offset is invalid: %s", optarg);
> +                     if (skip < 0)
> +                             errx(1, "offset is too small: %s", optarg);
> +                     if (skip == LLONG_MAX && errno == ERANGE)
> +                             errx(1, "offset is too large: %s", optarg);
> +                     unknown = 0;
>                       switch(*p) {
> +                     case '\0':
> +                             multiplier = 1;
> +                             break;
>                       case 'b':
> -                             skip *= 512;
> +                             multiplier = 512;
>                               break;
>                       case 'k':
> -                             skip *= 1024;
> +                             multiplier = 1024;
>                               break;
>                       case 'm':
> -                             skip *= 1048576;
> +                             multiplier = 1048576;
>                               break;
> +                     default:
> +                             unknown = 1;
>                       }
> +                     if (unknown || (p[0] != '\0' && p[1] != '\0'))
> +                             errx(1, "offset has invalid unit: %s", optarg);
> +                     if (skip > LLONG_MAX / multiplier)
> +                             errx(1, "offset is too large: %s", optarg);
> +                     skip *= multiplier;
>                       break;
>               case 'N':
> -                     if ((length = atoi(optarg)) < 0)
> -                             errx(1, "%s: bad length value", optarg);
> +                     errno = 0;
> +                     length = strtoll(optarg, &p, 0);
> +                     if (optarg[0] == '\0' || *p != '\0')
> +                             errx(1, "length is invalid: %s", optarg);
> +                     if (length < 0)
> +                             errx(1, "length is too small: %s", optarg);
> +                     if (length == LLONG_MAX && errno == ERANGE)
> +                             errx(1, "length is too large: %s", optarg);
>                       break;
>               case 'O':
>                       odadd("4/4 \"    %011o \" \"\\n\"");
> @@ -363,7 +389,7 @@ odoffset(int argc, char ***argvp)
>               base = 10;
>       }
>  
> -     skip = strtol(num, &end, base ? base : 8);
> +     skip = strtoll(num, &end, base ? base : 8);
>  
>       /* if end isn't the same as p, we got a non-octal digit */
>       if (end != p) {
> 

Reply via email to