From: Phillip Wood Date: Mon, 14 Oct 2024 14:55:18 GMT Subject: Re: [PATCH 3/3] parse: replace atoi() with strtoul_ui() and strtol_i() Message-ID: <2160f8ea-5f00-49d9-8e02-d71d4d827d39@gmail.com> In-Reply-To: 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 >>>> wrote: >>>>> >>>>> From: Usman Akinyemi >>>>> >>>>> 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 >