Re: [PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i()

> On Mon, Oct 14, 2024 at 9:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 13/10/2024 00:09, Usman Akinyemi via GitGitGadget wrote:
>>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>>>                }
>>>                if (skip_prefix(arg, "--max-connections=", &v)) {
>>> -                     max_connections = atoi(v);
>>> -                     if (max_connections < 0)
>>> -                             max_connections = 0;            /* unlimited */
>>> +                     if (strtol_i(v, 10, &max_connections) != 0 || max_connections < 0) {
>>
>> This is a faithful translation but if the aim of this series is to
>> detect errors then I think we want to do something like
>>
>>          if (strtol_i(v, 10, &max_connections))
>>                  die(...)
> ohh, what I understand in this part of the code is intended to set
> max_connections to 0 if the value it is currently set to is invalid,
> such as containing letters or being negative. Your suggestion implies
> that we should return an error to indicate that letters are not
> accepted.

>>          if (max_connections < 0)
>>                  max_connections = 0; /* unlimited */
>>
>>> +                             max_connections = 0;  /* unlimited */
>>> +                     }
>>>                        continue;
>>>                }
>>>                if (!strcmp(arg, "--strict-paths")) {
>>> diff --git a/imap-send.c b/imap-send.c
>>> index ec68a066877..33b74dfded7 100644
>>> --- a/imap-send.c
>>> +++ b/imap-send.c
>>> @@ -668,12 +668,12 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
>>>                return RESP_BAD;
>>>        }
>>>        if (!strcmp("UIDVALIDITY", arg)) {
>>> -             if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg))) {
>>> +             if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &ctx->uidvalidity) != 0) {
>>
>> The original is checking for a zero return from atoi() which indicates
>> an error or that the parsed value was zero. To do that with strtol_i()
>> we need to do
>>
>>          || (strtol_i(arg, 10, &ctx->uidvalidity) || !ctx->uidvalidity)
>>
>> The IMAP RFC[1] specifies that UIDVALIDITY should be a non-zero,
>> non-negative 32bit integer but I'm not sure we want to start change it's
>> type and using strtoul_ui here.
>>
>> [1] https://www.rfc-editor.org/rfc/rfc3501#section-2.3.1.1
>>
>>>                        fprintf(stderr, "IMAP error: malformed UIDVALIDITY status\n");
>>>                        return RESP_BAD;
>>>                }
>>>        } else if (!strcmp("UIDNEXT", arg)) {
>>> -             if (!(arg = next_arg(&s)) || !(imap->uidnext = atoi(arg))) {
>>> +             if (!(arg = next_arg(&s)) || strtol_i(arg, 10, &imap->uidnext) != 0) {
>>
>> The comments above apply here
>>
>>>                        fprintf(stderr, "IMAP error: malformed NEXTUID status\n");
>>>                        return RESP_BAD;
>>>                }
>>> @@ -686,8 +686,8 @@ static int parse_response_code(struct imap_store *ctx, struct imap_cmd_cb *cb,
>>>                for (; isspace((unsigned char)*p); p++);
>>>                fprintf(stderr, "*** IMAP ALERT *** %s\n", p);
>>>        } else if (cb && cb->ctx && !strcmp("APPENDUID", arg)) {
>>> -             if (!(arg = next_arg(&s)) || !(ctx->uidvalidity = atoi(arg)) ||
>>> -                 !(arg = next_arg(&s)) || !(*(int *)cb->ctx = atoi(arg))) {
>>> +             if (!(arg = next_arg(&s)) || (strtol_i(arg, 10, &ctx->uidvalidity) != 0) ||
>>> +                     !(arg = next_arg(&s)) || (strtol_i(arg, 10, (int *)cb->ctx) != 0)) {
>>
>> And here
>>
>>>                        fprintf(stderr, "IMAP error: malformed APPENDUID status\n");
>>>                        return RESP_BAD;
>>>                }
>>> @@ -773,7 +773,10 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
>>>                        if (!tcmd)
>>>                                return DRV_OK;
>>>                } else {
>>> -                     tag = atoi(arg);
>>> +                     if (strtol_i(arg, 10, &tag) != 0) {
>>
>> To check for an error just use (strtol_i(arg, 10, &tag))
>>
>>> +                             fprintf(stderr, "IMAP error: malformed tag %s\n", arg);
>>> +                             return RESP_BAD;
>>
>> This matches the error below so I assume it's good.
>>
>>> +                     }
>>>                        for (pcmdp = &imap->in_progress; (cmdp = *pcmdp); pcmdp = &cmdp->next)
>>>                                if (cmdp->tag == tag)
>>>                                        goto gottag;
>>> diff --git a/merge-ll.c b/merge-ll.c
>>> index 8e63071922b..2bfee0f2c6b 100644
>>> --- a/merge-ll.c
>>> +++ b/merge-ll.c
>>> @@ -427,8 +427,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
>>>        git_check_attr(istate, path, check);
>>>        ll_driver_name = check->items[0].value;
>>>        if (check->items[1].value) {
>>> -             marker_size = atoi(check->items[1].value);
>>> -             if (marker_size <= 0)
>>> +             if (strtol_i(check->items[1].value, 10, &marker_size) != 0 || marker_size <= 0)
>>
>> Here I think we want to return an error if we cannot parse the marker
>> size and then set the default if the marker size is <= 0 like we do for
>> the max_connections code in daemon.c above.
>>
>>>                        marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>>>        }
>>>        driver = find_ll_merge_driver(ll_driver_name);
>>> @@ -454,8 +453,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)
>>>                check = attr_check_initl("conflict-marker-size", NULL);
>>>        git_check_attr(istate, path, check);
>>>        if (check->items[0].value) {
>>> -             marker_size = atoi(check->items[0].value);
>>> -             if (marker_size <= 0)
>>> +             if (strtol_i(check->items[0].value, 10, &marker_size) != 0 || marker_size <= 0)
>>
>> And the same here
>>
>> Thanks for working on this, it will be a useful improvement to our
>> integer parsing. I think you've got the basic idea, it just needs a bit
>> of polish
>>
>> Phillip
>>