From: Sören Krecker Date: Fri, 27 Dec 2024 16:35:56 GMT Subject: Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc Message-ID: <965ac9bd-7340-4dbd-88da-2daa88c126c4@freenet.de> In-Reply-To: Hi everyone, If I understand your comments correctly, it would be preferably to switch to a data type like uint32_t or uint64_t so that the behavior is consisted on all platforms? Also add a test if the input overflows the data type. Best regards, Sören Krecker Junio C Hamano writes: > Phillip Wood writes: > >>>> struct hunk_header { >>>> - unsigned long old_offset, old_count, new_offset, new_count; >>>> + size_t old_offset, old_count, new_offset, new_count; >>> These are not "size"s in the traditional sense of what size_t is >>> (i.e. the number of bytes in a region of memory), but are more or >>> less proportional to that in that they count in number of lines. >>> If ulong is sufficient to count number of lines in an incoming >>> patch, then turning size_t may be excessive---are we sure that we >>> are not unnecessarily using wider-than-necessary size_t in some >>> places to hold these values for which ulong is sufficient, causing >>> compilers to emit unnecessary warning? >> >> That's my thought too - I think something like the diff below should >> fix the warnings by using more appropriate types in expressions >> involving the hunk header offset and count. Our internal diff >> implementation will not generate diffs for blobs greater than ~1GB >> and I don't think "git apply" can handle diff headers that contain >> numbers greater that ULONG_MAX so switching to size_t here seems >> unnecessary. > > Yes, exactly. > > Of course, when filling old_offset and friends by parsing an input > line like this: > > @@ -253,7 +253,7 @@ struct hunk_header { > > it would be a bug if we did not check if "253" overflows the type of > old_offset, etc. And I would very much welcome patches to fix such > a careless input validation routine. But replacing ulong with size_t > would not make such a problem go away. > > Now, I would be a bit more sympathetic if the patch were to use > integers of exact sizes, in the name of "let's make sure that > regardless of the platforms we handle patches up to the same limit". > But size_t is not a type that is appropriate for that (and of course > ulong is not, either---but the original did not aim for such a uniform > limit to begin with). > >> @@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s, >> else >> err(s, Q_("Sorry, only %d hunk available.", >> "Sorry, only %d hunks available.", >> - file_diff->hunk_nr), >> + (int)file_diff->hunk_nr), >> (int)file_diff->hunk_nr); >> } else if (s->answer.buf[0] == '/') { >> regex_t regex; > > I skimmed your "how about going this way" illustration patch and > found all the hunks reasonable, but this one I am not sure. Is > there a reason why hunk_nr has to be of type size_t? > > When queuing a hunk (and performing an operation that changes the > number of hunks, like splitting an existing one), the code should be > careful not to make too many hunks to overflow "int" (if that is the > more natural type to count them---and "int" being the most natural > integer type for the platform, I tend to think it should be fine), > again, that applies equally if the type of hunk_nr is "size_t". > > Thanks.