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

[Usman Akinyemi <usmanakinyemi202@gmail.com>]

On Mon, Oct 14, 2024 at 2:55 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 14/10/2024 15:00, Patrick Steinhardt wrote:
> > On Mon, Oct 14, 2024 at 02:57:13PM +0100, Phillip Wood wrote:
> >> On 14/10/2024 11:53, Patrick Steinhardt wrote:
> >>> On Sun, Oct 13, 2024 at 09:42:41AM +0000, Usman Akinyemi wrote:
> >>>> On Sat, Oct 12, 2024 at 11:09 PM Usman Akinyemi via GitGitGadget
> >>>> <gitgitgadget@gmail.com> wrote:
> >>>>>
> >>>>> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> >>>>>
> >>>>> Replace unsafe uses of atoi() with strtoul_ui() for unsigned integers
> >>>>> and strtol_i() for signed integers across multiple files. This change
> >>>>> improves error handling and prevents potential integer overflow issues.
> >>>>>
> >>>>> The following files were updated:
> >>>>> - daemon.c: Update parsing of --timeout, --init-timeout, and
> >>>>>     --max-connections
> >>>>> - imap-send.c: Improve parsing of UIDVALIDITY, UIDNEXT, APPENDUID, and
> >>>>>     tags
> >>>>> - merge-ll.c: Enhance parsing of marker size in ll_merge and
> >>>>>     ll_merge_marker_size
> >>>
> >>> To me it's always an indicator that something should be split up across
> >>> multiple commits once you have a bulleted list of changes in your commit
> >>> message.
> >>
> >> Agreed, but I think in this case there is a common theme (converting atoi()
> >> to a safer alternative) and the problem is with the commit message listing
> >> which files have changed rather than unrelated code changes being grouped
> >> together. This patch could be split up and if there were many more atoi()
> >> conversions it would need to be split to prevent it being too long but I
> >> don't think its essential to do so.
> >
> > In theory I agree. In practice I think we should have better
> > explanations why the respective conversions are fine and whether this is
> > fixing a bug or not. And if it is fixing bugs I'd also like to see tests
> > added to the tree.
>
> I'm not sure if I would describe any of the changes as fixing bugs. The
> option and config parsing code becomes stricter so I guess you could say
> it was a bug to accept any old rubbish and treat it as zero before. The
> imap code that's changed all rejected zero anyway apart from the tag
> parsing so maybe accepting the changes to the tag parsing are fixing a bug.
>
> > And by the time we got there it makes sense to split up commits.
>
> Yes if we start adding tests then it is worth splitting them up, I'm not
> sure we have anyway of testing the imap changes but it would be worth
> testing the other changes though.
>
> Phillip
>
> > Patrick
> >
>
I got this from a leftoverbit which the main issue was reported as
bug. https://public-inbox.org/git/CAC4O8c-nuOTS=a0sVp1603KaM2bZjs+yNZzdAaa5CGTNGFE7hQ@mail.gmail.com/

For the test, I should have the test as another patch right ?
Thanks.