On Wed, May 29, 2024 at 10:25:47AM -0700, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

+	char refspec_str[] = "refs/heads/*";
 
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.force = 0;
 	refspec.pattern = 1;
-	refspec.src = refspec.dst = "refs/heads/*";
+	refspec.src = refspec.dst = refspec_str;
 	get_fetch_map(remote_refs, &refspec, &fetch_map_tail, 0);
 	matches = guess_remote_head(find_ref_by_name(remote_refs, "HEAD"),
 				    fetch_map, 1);

I have no objections to changes along this line, but this makes me
wonder if we somehow have ways to ensure that after everything is
done, refspec_str[], empty_str[], and the like in other hunks remain
to have their initial contents.  Stepping back a bit, without
applying this series, if we told the compiler to store these
constant strings in read-only segment, any attempt to write into
refspec.src or refspec.dst string would have been caught at runtime
as an error.  With the patch, that would no longer work.  The piece
of memory in refspec_str[] is a fair game to be overwritten by
anybody.

True. Ideally, we wouldn't have to do these acrobatics here in the first
place. That would likely require quite a lot more work and go beyond the
scope of this patch series.

Which makes me wonder why these refspec.src and refspec.dst members
are not "const char *" pointers in the first place?  Obviously we do
not expect "refs/heads/*" to be overwritten after storing the pointer
to it in these members and making the get_fetch_map() call.

Well, we do. Not in `get_fetch_map()`, but in `query_refspecs()`. It
does weird stuff where it writes the result into either `src` or `dst`
depending on which of these fields is provided by the caller. Which
means that one of the fields would typically be a constant, whereas the
other one will be allocated.

Really, once you start looking you find all kinds of weird interfaces
where we cannot quite make up our mind whether fields are controlled by
the caller or by the callee. This patch series certainly surfaces quite
some places that left me scratching my head.

Patrick