patch, 14 partscacheinfo_callback(): use convert_ui() when handling "--cacheinfo"

Sorry for the long email. Feel free to skip over the background info and continue reading at "My solution" below.

This odyssey started with a typo:

    $ git shortlog -snl v2.2.0..v2.3.0
     14795      Junio C Hamano
      1658      Jeff King
      1400      Shawn O. Pearce
      1109      Linus Torvalds
       760      Jonathan Nieder
    [...]

It took me a minute to realize why so many commits are listed. It turns out that "-l", which I added by accident, requires an integer argument, but it happily swallows "v2.2.0..v2.3.0" without error. This leaves no range argument, so the command traverses the entire history of HEAD.

The relevant code is

        else if ((argcount = short_opt('l', av, &optarg))) {
                options->rename_limit = strtoul(optarg, NULL, 10);
                return argcount;
        }

, which is broken in many ways. First of all, strtoul() is way too permissive for simple tasks like this:

* It allows leading whitespace.

* It allows arbitrary trailing characters.

* It allows a leading sign character ('+' or '-'), even though the
  result is unsigned.

* If the string doesn't contain a number at all, it sets its "endptr"
  argument to point at the start of the string but doesn't set errno.

* If the value is larger than fits in an unsigned long, it returns the
  value clamped to the range 0..ULONG_MAX (setting errno to ERANGE).

* If the value is between -ULONG_MAX and 0, it returns the positive
  integer with the same bit pattern, without setting errno(!) (I can
  hardly think of a scenario where this would be useful.)

* For base=0 (autodetect base), it allows a base specifier prefix "0x"
  or "0" and parses the number accordingly. For base=16 it also allows
  a "0x" prefix.

strtol() has similar foibles.

The caller compounds the permissivity of strtoul() with further sins:

* It does absolutely no error detection.

* It assigns the return value, which is an unsigned long, to an int
  value. This could truncate the result, perhaps even resulting in
  rename_limit being set to a negative value.

When I looked around, I found scores of sites that call atoi(), strtoul(), and strtol() carelessly. And it's understandable. Calling any of these functions safely is so much work as to be completely impractical in day-to-day code.

git-compat-util.h has two functions, strtoul_ui() and strtol_i(), that try to make parsing integers a little bit easier. But they are only used in a few places, they hardly restrict the pathological flexibility of strtoul()/strtol(), strtoul_ui() doesn't implement clamping consistently when converting from unsigned long to unsigned int, and neither function can be used when the integer value *is* followed by other characters.

My solution: the numparse module

So I hereby propose a new module, numparse, to make it easier to parse integral values from strings in a convenient, safe, and flexible way. The first commit introduces the new module, and subsequent commits change a sample (a small fraction!) of call sites to use the new functions. Consider it a proof-of-concept. If people are OK with this approach, I will continue sending patches to fix other call sites. (I already have another two dozen such patches in my repo).

Please see the docstrings in numparse.h in the first commit for detailed API docs. Briefly, there are eight new functions:

    parse_{l,ul,i,ui}(const char *s, unsigned int flags,
                      T *result, char **endptr);
    convert_{l,ul,i,ui}(const char *s, unsigned int flags, T *result);

The parse_*() functions are for parsing a number from the start of a string; the convert_*() functions are for parsing a string that consists of a single number. The flags argument selects not only the base of the number, but also which of strtol()/strtoul()'s many features should be allowed. The *_i() and *.ui() functions are for parsing int and unsigned int values; they are careful about how they truncate the corresponding long values. The functions all return 0 on success and a negative number on error.

Here are a few examples of how these functions can be used (taken from the header of numparse.h):

* Convert hexadecimal string s into an unsigned int. Die if there are
  any characters in s besides hexadecimal digits, or if the result
  exceeds the range of an unsigned int:

    if (convert_ui(s, 16, &result))
            die("...");

* Read a base-ten long number from the front of a string, allowing
  sign characters and setting endptr to point at any trailing
  characters:

    if (parse_l(s, 10 | NUM_SIGN | NUM_TRAILING, &result, &endptr))
            die("...");

* Convert decimal string s into a signed int, but not allowing the
  string to contain a '+' or '-' prefix (and thereby indirectly
  ensuring that the result will be non-negative):

    if (convert_i(s, 10, &result))
            die("...");

* Convert s into a signed int, interpreting prefix "0x" to mean
  hexadecimal and "0" to mean octal. If the value doesn't fit in an
  unsigned int, set result to INT_MIN or INT_MAX.

    if (convert_i(s, NUM_SLOPPY, &result))
            die("...");

My main questions:

* Do people like the API? My main goal was to make these functions as
  painless as possible to use correctly, because there are so many
  call sites.

* Is it too gimmicky to encode the base together with other options in
  `flags`? (I think it is worth it to avoid the need for another
  parameter, which callers could easily put in the wrong order.)

* Am I making callers too strict? In many cases where a positive
  integer is expected (e.g., "--abbrev=<num>"), I have been replacing
  code like

      result = strtoul(s, NULL, 10);

  with

      if (convert_i(s, 10, &result))
              die("...");

  Strictly speaking, this is backwards incompatible, because the
  former allows things like

      --abbrev="+10"
      --abbrev="    10"
      --abbrev="10 bananas"
      --abbrev=-18446744073709551606

  (all equivalent to "--abbrev=10"), whereas the new code rejects all
  of them. It would be easy to change the new code to allow the first
  three, by adding flags NUM_PLUS, NUM_LEADING_WHITESPACE, or
  NUM_TRAILING respectively. But my inclination is to be strict
  (though I could easily be persuaded to permit the first one).

* Should I submit tiny patches, each rewriting a single call site, or
  make changes in larger chunks (say, file by file)? For example, in
  revision.c there is a function handle_revision_opt() that contains
  about 10 call sites that should be rewritten, for 10 different
  options that take integers. My gut feeling is that the rewrite of
  "--min-age=<value>" should be done in a different patch than that of
  "--min-parents=<value>", especially since the invocations might use
  different levels of parsing strictness that might be topics of
  discussion on the ML. On the other hand, I feel silly bombarding the
  list with tons of tiny patches.

* I couldn't think of any places where today's sloppy parsing could
  result in security vulnerabilities, but people should think about
  this, too. I would be especially wary of sites that call strtoul()
  and assign the result to an int, seemingly not considering that the
  result could end up negative.

These patches apply to "master". They are also available on my GitHub repo [1].

Michael

[1] https://github.com/mhagger/git.git, branch "numparse1"

Michael Haggerty (14):
  numparse: new module for parsing integral numbers
  cacheinfo_callback(): use convert_ui() when handling "--cacheinfo"
  write_subdirectory(): use convert_ui() for parsing mode
  handle_revision_opt(): use skip_prefix() in many places
  handle_revision_opt(): use convert_i() when handling "-<digit>"
  strtoul_ui(), strtol_i(): remove functions
  handle_revision_opt(): use convert_ui() when handling "--abbrev="
  builtin_diff(): detect errors when parsing --unified argument
  opt_arg(): val is always non-NULL
  opt_arg(): use convert_i() in implementation
  opt_arg(): report errors parsing option values
  opt_arg(): simplify pointer handling
  diff_opt_parse(): use convert_i() when handling "-l<num>"
  diff_opt_parse(): use convert_i() when handling --abbrev=<num>

 Makefile                                  |   1 +
 builtin/update-index.c                    |   3 +-
 contrib/convert-objects/convert-objects.c |   3 +-
 diff.c                                    |  55 ++++----
 git-compat-util.h                         |  26 ----
 numparse.c                                | 180 ++++++++++++++++++++++++++
 numparse.h                                | 207 ++++++++++++++++++++++++++++++
 revision.c                                |  64 ++++-----
 8 files changed, 447 insertions(+), 92 deletions(-)
 create mode 100644 numparse.c
 create mode 100644 numparse.h

Implement wrappers for strtol() and strtoul() that are safer and more convenient to use.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Makefile   |   1 +
 numparse.c | 180 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 numparse.h | 207 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 388 insertions(+)
 create mode 100644 numparse.c
 create mode 100644 numparse.h

On Tuesday, March 17, 2015, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Perhaps use a /* one-line style comment */ to reduce vertical space consumption a bit, thus make it (very slightly) easier to run the eye over the code.

Would it reduce cognitive load slightly (and reduce vertical space consumption) to rephrase the conditionals as:

    if (errno == ERANGE && !(flags & NUM_SATURATE))
        return -NUM_SATURATE;

    if (errno && errno != ERANGE)
        return -NUM_OTHER_ERROR;

or something similar?

More below.

Did you mean s/unsigned int/signed int/ ?

Does this restriction go against the goal of making these functions convenient, even while remaining strict? Is there a strong reason for not merely inferring NUM_BASE_SPECIFIER when base is 0? Doing so would make it consistent with strto*l() without (I think) introducing any ambiguity.

"(see)" what?

Again, on the subject of convenience, why this restriction? The stated purpose of the parse_*() functions is to parse the number from the front of the string and return a pointer to the first non-numeric character following. As a reader of this API, I interpret that as meaning that NUM_TRAILING is implied. Is there a strong reason for not inferring NUM_TRAILING for the parse_*() functions at the API level? (I realize that the convert_*() functions are built atop parse_*(), but that's an implementation detail.)

Do we want to perpetuate the ugly (char **) convention for 'endptr' from strto*l()? Considering that the incoming string is const, it seems undesirable to return a non-const pointer to some place inside that string.

I can formulate arguments for allowing or disallowing NUM_TRAILING with convert_*(), however, given their purpose of parsing the entire string into a number, as a reader of the API, I would kind of expect the convert_*() functions to ensure that NUM_TRAILING is not set (either by forcibly clearing it or erroring out as an inconsistent request if it is set).

On 03/18/2015 07:27 PM, Eric Sunshine wrote:

Yes, will change.

The most common case by far should be that errno is zero. The code as written only needs one test for that case, whereas your code needs two tests. I think it is worth compromising on code clarity a tiny bit to avoid the extra test.

Thanks for catching this. I will fix it.

I thought about doing this. If it were possible to eliminate NUM_BASE_SPECIFIER altogether, then there is no doubt that it would be a good change. But NUM_BASE_SPECIFIER also has an effect when base==16; namely, that an "0x" prefix, if present, is consumed. So

    parse_i("0xff", 16 | NUM_BASE_SPECIFIER, &result, &endptr)

gives result==255 and endptr==s+4, whereas

    parse_i("0xff", 16, &result, &endptr)

gives result==0 and entptr==s+1 (it treats the "x" as the end of the string).

We could forgo that feature, effectively allowing a base specifier if and only if base==0. But I didn't want to rule out allowing an optional base specifier for base==16, in which case NUM_BASE_SPECIFIER can't be dispensed with entirely.

If you agree with that, then the remaining question is: which policy is less error-prone? My thinking was that forcing the caller to specify NUM_BASE_SPECIFIER explicitly when they select base==0 will serve as a reminder that the two features are intertwined. Because another imaginable policy (arguably more consistent with the policy for base!=0) would be that

    convert_i(s, 0, &result)

, because it *doesn't* specify NUM_BASE_SPECIFIER, doesn't allow a base prefix, and therefore indirectly only allows base-10 numbers.

But I don't feel strongly about this.

That was meant to be a pointer to the documentation for the NUM_* constants. But obviously it was too oblique, so I will make it more explicit, maybe

    ... and the NUM_* constants documented above.

Yes, I'd also thought about that change:

* Make NUM_TRAILING private.
* Make the current parse_*() functions private.
* Add new parse_*(s, flags, result, endptr) functions that imply
NUM_TRAILING (and maybe they should *require* a non-null endptr argument).
* Change the convert_*() to not allow the NUM_TRAILING flag.

This would add a little bit of code, so I didn't do it originally. But since you seem to like the idea too, I guess I will make the change.

Yes, I guess we could do some internal casting and expose endptr as (const char **). It would make it a bit harder if the user actually wants to pass a non-const string to the function and then modify the string via the returned endptr, but I haven't run into that pattern yet so let's make the change you suggested.

Yes, I think I will change this, as discussed above.

Thanks for your careful and thoughtful comments! Michael

On Wed, Mar 18, 2015 at 6:47 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Making base==0 a special case doesn't have to mean ruling out or eliminating NUM_BASE_SPECIFIER for the base==16 case. However, a special case base==0 would make the API a bit non-orthogonal and require extra documentation. But for those familiar with how strto*l() treats base==0 specially, the rule of least surprise may apply.

Since base==0 would unambiguously imply NUM_BASE_SPECIFIER, being able to tersely say convert_i(s, 0, &result) would be a win from a convenience perspective. However...

I don't feel strongly about it either, and can formulate arguments either way. Assuming you stick with your current design, if the strictness of having to specify NUM_BASE_SPECIFIER for base==0 proves too burdensome, it can be loosened later by making base==0 a special case.

On the other hand, if you go with Junio's suggestion of choosing names for these functions which more closely mirror strto*l() names, then the rule of least surprise might suggest that a special case for base==0 has merit.

The other option (as Junio also suggested) is to collapse this API into just the four parse_*() functions. All of the call-sites you converted in this series invoked convert_*(), but it's not clear that having two sets of functions which do almost the same thing is much of a win. If the default is for NUM_TRAILING to be off, and if NULL 'endptr' is allowed, then:

    parse_ul(s, 10, &result, NULL);

doesn't seem particularly burdensome compared with:

    convert_ul(s, 10, &result);

A more compact API, with only the parse_*() functions, means less to remember and less to document, and often is more orthogonal.

The argument for dropping the convert_*() functions is strengthened if you follow Junio's suggestion to name these functions after the strto*l() variations.

I don't feel strongly about this either. It always struck me as a bit of an API wart, but I can see the convenience value of non-const 'endptr' if the caller intends to modify the string. If you rename these functions to mirror strto*l(), then the rule of least surprise would suggest that 'endptr' should remain non-const.

Michael Haggerty <mhagger@alum.mit.edu> writes:

I somehow feel that a pre-processing that only _inspects_ part of the string, without munging that string (e.g. notice '-' but feed that to underlying strtol(3)) somewhat a brittle approach. When I read the above for the first time, I somehow expected that the code would notice leading '-', strip that leading '-' and remember the fact that it did so in the *flags, let the strtol(3) to parse the remainder _and_ always make sure the returned result is not negative (because that would imply that the original input had two leading minuses and digits), and give the sign based on what this preprocess found out in *flags, and then seeing that there is no sign of such processing in the caller I scratched my head.

I still have not convinced myself that what I am seeing in the base==16 part in the above is correct.

Use convert_ui() instead of strtoul_ui() to parse the <mode> argument. This tightens up the parsing a bit:

* Leading whitespace is no longer allowed
* '+' and '-' are no longer allowed

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/update-index.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Use convert_ui() instead of strtoul_ui() when parsing tree entries' modes. This tightens up the parsing a bit:

* Leading whitespace is no longer allowed
* '+' and '-' are no longer allowed

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 contrib/convert-objects/convert-objects.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

This reduces the need for "magic numbers".

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 revision.c | 59 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

This tightens up the parsing a bit:

* Leading whitespace is no longer allowed
* '+' and '-' are no longer allowed

It also removes the need to check separately that max_count is non-negative.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Michael Haggerty <mhagger@alum.mit.edu> writes:

Hmmm, on the surface this sound nice, but I am not sure if it is a good idea. "git cmd-in-log-family -3 --max-count=-1" would have been a nice way to flip a max-count given earlier on the command line back to the default (unlimited) and we are removing it without giving any escape hatch?

Their callers have been changed to use the numparse module.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 git-compat-util.h | 26 --------------------------
 1 file changed, 26 deletions(-)

This adds error checking, where previously there was none. It also disallows '+' and '-', leading whitespace, and trailing junk.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 revision.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

The previous code used strtoul() without any checks that it succeeded. Instead use convert_l(), in strict mode, and die() if there is an error. This tightens up the parsing:

* Leading whitespace is no longer allowed
* '+' and '-' are no longer allowed
* Trailing junk is not allowed

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 diff.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

opt_arg() is never called with val set to NULL, so remove the code for handling that eventuality (which anyway wasn't handled consistently in the function).

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This shortens the code and avoids the old code's careless truncation from unsigned long to int.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 diff.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

If an argument is there, but it can't be parsed as a non-positive number, then die() rather than returning 0.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Increment "arg" when a character is consumed, not just before consuming the next character.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 diff.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

die() with an error message if the argument is not a non-negative integer. This change tightens up parsing: '+' and '-', leading whitespace, and trailing junk are all disallowed now.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 diff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

die() with an error message if the argument is not a non-negative integer. This change tightens up parsing: '+' and '-', leading whitespace, and trailing junk are all disallowed now.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 diff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Michael Haggerty <mhagger@alum.mit.edu> writes:

Everybody leading up to this step said "a non-negative integer", but this one is different?

Michael Haggerty <mhagger@alum.mit.edu> writes:

Exactly. They were introduced to prevent sloppy callers from passing NULL to the &end parameter to underlying strtoul(3). The first thing that came to my mind while reading your message up to this point was "why not use them more, tighten them, adding different variants if necessary, instead of introducing an entirely new set of functions in a new namespace?"

For example:

I think strictness would be good and those who did "--abbrev=' 20'" deserve what they get from the additional strictness, but

	if (strtol_i(s, 10, &result))

would have been more familiar.

On 03/17/2015 07:48 PM, Junio C Hamano wrote:

Here were my thoughts:

* I wanted to change the interface to look less like
  strtol()/strtoul(), so it seemed appropriate to make the names
  more dissimilar.

* The functions seemed long enough that they shouldn't be inline,
  and I wanted to put them in their own module rather than put
  them in git-compat-util.h.

* It wasn't obvious to me how to generalize the names, strtoul_ui()
  and strtol_i(), to the eight functions that I wanted.

That being said, I'm not married to the names. Suggestions are welcome--but we would need names for 8 functions, not 4 [1].

Michael

[1] It could be that when we're done, it will turn out that some of the eight variants are not needed. If so, we can delete them then.

Michael Haggerty <mhagger@alum.mit.edu> writes:

One reason I prefer the names in the compat-util is that it makes it clear that what is converted into what (e.g. "str" to "ul") and what type the result is stored ("i" or "ui"), and as an added bonus, with a short-and-sweet "to" it is already so clear we are "converting" that we do not have to use verbs like "parse_" or "convert_". From parse_i() and convert_i(), I cannot tell which one lacks the endptr and parses to the end and which one takes the endptr and tells the caller where the conversion ended (and it also does not tell us that they are converting from a string).

I actually agree on this, but that is not a justification why they have to be named very differently.

Having said that, I would say that it will be futile to discuss this further, as we seem to agree to disagree.

On Tue, Mar 17, 2015 at 11:00 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Thank you for doing it. I was about to write another number parser and you did it :D Maybe you can add another patch to convert the only strtol in upload-pack.c to parse_ui. This place should accept positive number in base 10, plus sign is not accepted.

On 03/18/2015 12:05 AM, Duy Nguyen wrote:

If the general direction of this patch series is accepted, I'll gradually try to go through the codebase, replacing *all* integer parsing with these functions. So there's no need to request particular callers of strtol()/strtoul() to be converted; I'll get to them all sooner or later (I hope).

But in case you have some reason that you want upload-pack.c to be converted right away, I just pushed that change (plus some related cleanups) to my GitHub repo [1]. The branch depends only on the first patch of the "numparse" patch series.

By the way, some other packet line parsing code in that file doesn't verify that there are no trailing characters on the lines that they process. That might be another thing that should be tightened up.

Michael

[1] https://github.com/mhagger/git.git, branch "upload-pack-numparse"

On Wed, Mar 18, 2015 at 4:47 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Good to know. No there's no hurry in converting this particular place. It's just tightening things up, not bug fixing or anything.

On Wed, Mar 18, 2015 at 10:47:40AM +0100, Michael Haggerty wrote:

Do you mean that upload-pack gets a pkt-line of length N that contains a line of length M, and then doesn't check that M==N? We use the space between M and N for passing capabilities and other metadata around.

Or do you mean that we see lines like:

  want [0-9a-f]{40} ...\n

and do not bother looking at the "..." that comes after the data we expect? That I can believe, and I don't think it would hurt to tighten up (we shouldn't need it for extensibility, as anybody trying to stick extra data there should do so only after using a capability flag earlier in the protocol).

-Peff

On 03/18/2015 11:03 AM, Jeff King wrote:

The latter. For example here [1], the "have" command and its SHA-1 are read from the line, but I don't see a check that there are no characters after the SHA-1. The same here [2].

Michael

[1] https://github.com/gitster/git/blob/9ab698f4000a736864c41f57fbae1e021ac27799/upload-pack.c#L404-L429 [2] https://github.com/gitster/git/blob/9ab698f4000a736864c41f57fbae1e021ac27799/upload-pack.c#L550-L565

On Tue, Mar 17, 2015 at 05:00:02PM +0100, Michael Haggerty wrote:

I definitely like the overall direction of this. My first thought was that it does seem like there are a lot of possible options to the functions (and OR-ing the flags with the base does seem weird, though I can't think of a plausible case where it would actually cause errors). Many of those options don't seem used in the example conversions (I'm not clear who would want NUM_SATURATE, for example).

I wondered if we could do away with the radix entirely. Wouldn't we be asking for base 10 most of the time? Of course, your first few patches show octal parsing, but I wonder if we should actually have a separate parse_mode() for that, since that seems to be the general reason for doing octal parsing. 100000644 does not overflow an int, but it is hardly a reasonable mode.

I also wondered if we could get rid of NUM_SIGN in favor of just having the type imply it (e.g., convert_l would always allow negative numbers, whereas convert_ul would not). But I suppose there are times when we end up using an "int" to store an unsigned value for a good reason (e.g., "-1" is a sentinel value, but we expect only positive values from the user). So that might be a bad idea.

I notice that you go up to "unsigned long" here for sizes. If we want to use this everywhere, we'll need something larger for parsing off_t values on 32-bit machines (though I would not at all be surprised with the current code if 32-bit machines have a hard time configuring a pack.packSizeLimit above 4G).

I wonder how much of the boilerplate in the parse_* functions could be factored out to use a uintmax_t, with the caller just providing the range. That would make it easier to add new types like off_t, and possibly even constrained types (e.g., an integer from 0 to 100). On the other hand, you mentioned to me elsewhere that there may be some bugs in the range-checks of config.c's integer parsing. I suspect they are related to exactly this kind of refactoring, so perhaps writing things out is best.

IMHO most of the tightening happening here is a good thing, and means we are more likely to notice mistakes rather than silently doing something stupid.

For sites that currently allow it, I could imagine people using hex notation for some values, though, depending on the context. It looks there aren't many of them ((it is just when the radix is "0", right?). Some of them look to be accidental (does anybody really ask for --threads=0x10?), but others might not be (the pack index-version contains an offset field that might be quite big).

Feel free to ignore any or all of that. It is not so much criticism as a dump of thoughts I had while reading the patches. Perhaps you can pick something useful out of that, and perhaps not. :)

-Peff

Jeff King <peff@peff.net> writes:

True.

Yeah, there is a reason why ssize_t exists.

I do like the general idea of making sure we avoid use of bare atoi() and unchecked use of strtol() and such, and have a set of well defined helper functions to perform the common checks, exactly for the reason you said.

Jeff King <peff@peff.net> writes:

I like this idea very well. I wonder if we can implement the family of

    parse_{type}(const char *, unsigned int flags,
    		 const char **endptr, {type} *result)

functions by calls a helper that internally deals with the numbers in uintmax_t, and then checks if the value fits within the possible range of the *result before returning.

    int parse_i(const char *str, unsigned flags,
		const char **endptr, int *result) {
	uintmax_t val;
        int sign = 1, status;
        if (*str == '-') {
		sign = -1; 
                str++;
	}
        status = parse_num(str, flags, endptr, &val, INT_MAX);
        if (status < 0)
        	return status;
	*result = sign < 0 ? -val : val;
        return 0;
    }

(assuming the last parameter to parse_num is used to check if the result fits within that range). Or that may be easier and cleaner to be done in the caller with or something like that:

	status = parse_num(str, flags, endptr, &val);
        if (status < 0)
        	return status;
	if (INT_MAX <= val * sign || val * sign <= INT_MIN) {
        	errno = ERANGE;
                return -1;
	}

If we wanted to, we may even be able to avoid duplication of boilerplate by wrapping the above pattern within a macro, parameterizing the TYPE_{MIN,MAX} and using token pasting, to expand to four necessary result types.

There is no reason for the implementation of the parse_num() helper to be using strtoul(3) or strtoull(3); its behaviour will be under our total control. It can become fancier by enriching the flags bits (e.g. allow scaling factor, etc.) only once and all variants for various result types will get the same feature.

On 03/19/2015 08:32 AM, Junio C Hamano wrote:

Parsing numbers is not rocket science, but there are a lot of pitfalls, especially around overflow. It's even harder to write such code via macros and the result is less readable.

This patch series is mostly about finding a reasonable API and whipping the callers into shape. That seems ambitious enough for me. I'd rather stick with boring wrappers for now and lean on strtol()/strtoul() to do the dirty work. It will be easy for a motivated person to change the implementation later.

Michael

Am 24.03.2015 um 17:06 schrieb Michael Haggerty:

The OpenBSD developers invented strtonum for that.  Are you aware of it? 
  Would it fit?  This discussion may be useful:

	http://www.tedunangst.com/flak/post/the-design-of-strtonum

René

On 03/24/2015 05:49 PM, René Scharfe wrote:

I wasn't aware of strtonum; thanks for the reference. It has an untraditional interface. Their willingness to sacrifice half of the unsigned range and requirement that the user specify minval and maxval have the nice effect of permitting one function to be used for all integer types.

I think git will need more flexibility, for example to support other radixes and to allow trailing characters. So personally I don't think we should use (or imitate) strtonum().

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

[jc: dropped a non-working address from Cc list]

I had the same impression. An earlier suggestion by Peff about uintmax_t may be something we might want to keep in mind; we might want to use strtoll/strtoull as the underlying implementation if we wanted to avoid rolling our own.

Thanks.

On 03/19/2015 06:26 AM, Jeff King wrote:

There are a lot of options, but so far only few of them have been used. As the call sites are rewritten we will see which of these features are useful, and which can be dispensed with. If groups of options tend to be used together, we can define constants for them like I've done with NUM_SLOPPY and NUM_SIGN.

Regarding NUM_SATURATE: I'm not sure who would want it either, but I thought there might be places where the user wants to specify (effectively) "infinity", and it might be convenient to let him specify something easy to type like "--max=999999999999" rather than "--max=2147483647".

Again, as a first pass I wanted to just have a really flexible API so that call sites can be rewritten without a lot of extra thought. If somebody wants to add a parse_mode() function, it will be easy to build on top of convert_ui(). But that change can be done after this one.

Yes, as I was rewriting call sites, I found many that used the unsigned variants of the parsing functions but stored the result in an int. Probably some of these use -1 to denote "unset"; it might be that there are other cases where the variable could actually be declared to be "unsigned int".

Prohibiting signs when parsing signed quantities isn't really elegant from an API purity point of view, but it sure is handy!

Yes, probably. I haven't run into such call sites yet.

It's not a lot of code yet. If we find out we need variants for size_t and off_t and uintmax_t and intmax_t then such a refactoring would definitely be worth considering.

Yes, we can even debate whether we want to implement a general policy that user-entered integers can be specified in any radix. Probably nobody will ever specify "--threads=0x10", but is there harm in allowing it?

Against the gain in flexibility, I see the following potential disadvantages:

* Added cognitive burden for rarely-used flexibility.
* Other Git clients might feel obliged to be just as flexible, causing
their implementers extra work.
* Users might see commands written in unfamiliar ways (e.g. in scripts
or stackoverflow) and get confused.
* Octal is ambiguous with decimal. What to make of "--count=010"? People
who expect octal numbers to be allowed will think it means 8, whereas
people who don't expect octal numbers to be allowed (or don't even know
what an octal number is!) will think that it means 10. Such uses might
even already exist and have their meaning changed if we start allowing
octal.

All in all, I thought it is less error-prone to default to allowing only decimal, except in selected situations where hex and octal are traditionally used (e.g., file modes, memory limits).

Most of the call sites so far have explicitly specified decimal parsing, and I have left them unchanged.

Thanks very much for the feedback!

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

This might be blessing in disguise. Our friends on MacOS may be relying on that

    git cmd --abbrev="$(wc -c <foo)"

to work "as expected", even though their "wc" gives leading spaces, for example.

Which is why we have introduced strtoul_ui() and such.

I do not particularly see it a bad thing to accept "--abbrev=+7". Using unsigned type to accept a width and parsing "--abbrev=-7" into a large positive integer _is_ a problem, and we may want to fix it, but I think that is still within the scope of the original "better strtol/strtoul", and I do not see it as a justification to introduce a set of functions with completely unfamiliar name.

Why is that a problem? A caller that cares is supposed to check endptr and the string it gave, no? Now, if strtoul_ui() and friends do not do so, I again think that is still within the scope of the original "better strtol/strtoul".

Ditto.

Ditto.

Why is it a problem to allow "git cmd --hexval=0x1234", even if "git cmd --hexval=1234" would suffice?

Yes, these burdens on the caller were exactly why strtoul_ui() etc. wanted to reduce---and it will be a welcome change to do necessary checks that are currently not done.

I am not sure if I follow. Why not unify them into one 4-function set, with optional endptr that could be NULL?

While we are on the topic of improving number parsing, one thing that I found somewhat frustrating is that config.c layer knows the scaling suffix but option parsing layer does not. These functions should offer an option (via their "flags") to say "I allow scaled numbers like 2k, 4M, etc.".

On 03/19/2015 07:22 AM, Junio C Hamano wrote:

Yuck. If you'd like, I can make sure to continue allowing leading whitespace everywhere that it is allowed now. Alternatively, we could make the parsing tighter and see if anybody squeals. What do you think?

It is easy to allow "--abbrev=+7"; I would just need to add NUM_PLUS to those call sites. Should I do so?

It would even be possible to allow "--abbrev=-0", which is also a non-negative integer. But it's more work and it seems pretty silly to me.

The text you are quoting was my argument for why we need wrappers around strtol() and strtoul(), not for how the wrappers should be named.

The "endptr" convention for detecting errors is fine in theory, but in practice a large majority of callers didn't check for errors correctly. This is partly because the "endptr" convention is so awkward.

The existing strtoul_ui() and strtol_i() did check endptr correctly, but there were only int-sized variants of the functions, and they didn't give the caller the chance to capture endptr if the caller wanted to allow trailing characters.

Why did I rename the wrapper functions?

* The old names, I think, emphasize that they take the long-sized
results of strtou?l() and convert them to integer size, which I think is
an implementation detail.
* The new functions will also have long versions. The obvious way to
generalize the existing function names for long variants (srtoul_ul()
and strtol_l()) seem kindof redundant.
* I wanted to change the signature and behavior of the functions, so
knowledge of the existing functions wouldn't be super helpful in
understanding the new ones.

In some cases we would like to allow that flexibility; in some cases not. But the strtol()/strtoul() functions *always* allow it.

In the next version of this patch series I plan to include only four functions, str_to_{i,ui,l,ul}(), named that way to make them more reminiscent of the functions that they replace. They will take an entptr argument, but if that argument is set to NULL, then the function will error out if there are any trailing characters.

That's a good idea. But I think I will leave that for a later patch series, if that is OK with you.

Thanks for the great feedback! Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

The more relevant question to ask from my point of view is why you need to "add" NUM_PLUS to "enable" it. What valid reason do you have to forbid it anywhere? Only because you do not accept it by default, you need to "add" to "enable".

The same issue.  Whare are these "some cases"?

Junio C Hamano <gitster@pobox.com> writes:

And the same issue appears in the "leading whitespace" thing I did not mention in the earlier part of your message I responded to. I also notice you answered yourself that there may not be a valid reason to forbid end-user supplied "0x" prefix to arguments we expect an integer for in your other message.

In short, if it is not a clearly bogus input that indicates a typo or something (e.g. "--size=48l? did the user meant 48, 48k, or 48m?"), and if it is clear we can tell the user meant what the code would naturally interpret as (e.g. "--hexval=0x1234"), why forbid it?

On 03/24/2015 04:58 PM, Junio C Hamano wrote:

I want to be able to plunge into this project without first auditing all call sites to see which features will turn out to be needed. So I'm erring on the side of flexibility. For now, I want to be able to prohibit '+' signs.

Currently all of the flags cause additional features to be enabled. My guess was that most callers *won't* need most features, so it seemed easiest and most consistent to have all features be turned off by default and let the caller add the features that it wants to allow.

Regarding specifically allowing/disallowing a leading '+': I saw a couple of callsites that explicitly check that the first character is a digit before calling strtol(). I assumed that is to disallow sign characters [1]. For example,

    diff.c: optarg()
    builtin/apply.c: parse_num()
    maybe date.c: match_multi_number()

There are other callsites that call strtoul(), but store the result in a signed variable. Those would presumably not want to allow leading '-', but I'm not sure.

I also imagine that there are places in protocols or file formats where signs should not be allowed (e.g., timestamps in commits?).

I admit I'm not sure there are such places for hexadecimal numbers.

I'm coming around to an alternate plan:

Step 1: write a NUM_DEFAULT combination-of-options that gives the new functions behavior very like strtol()/strtoul() but without their insane features.

Step 2: rewrite all callers to use that option (and usually endptr=NULL, meaning no trailing characters) unless it is manifestly clear that they are already trying to forbid some other features. This will already produce the largest benefit: avoiding overflows, missing error checking, etc.

Steps 3 through ∞: as time allows, rewrite individual callsites to be stricter where appropriate.

Hopefully steps 1 and 2 will not be too controversial.

Michael

[1] That assumption is based on a rather quick look over the code, because with well over 100 callsites, it is not practical to study each callsite carefully.

Michael Haggerty <mhagger@alum.mit.edu> writes:

This one I know is from "if not a digit we know it is not a number"; it is not an attempt to say "we must forbid numbers to be spelled with '+'", but more about "we do not need it and this is easier to code without a full fledged str-to-num helper API" sloppiness.

This parses "@@ -l,k +m,n @@@" after stripping the punctuation around the numbers, so this is a valid reason why you would want an optional feature "CANNOT_HAVE_SIGN" in the str-to-num parser and use it.

The approxidate callers parse random garbage input and attempt to make best sense out of it, so they tend to try limitting the damage caused by incorrect guesses by insisting "we do not consider +0 to be zero" and such. So this is a valid reason why you would want an optional feature "CANNOT_HAVE_SIGN" in the str-to-num parser and use it.

All sounds sensible to me.