Re: [PATCH] apply: Avoid ambiguous pointer provenance for CHERI/Arm's Morello

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2022, 23:41 UTC
Message-ID: <xmqqwnjca0vn.fsf@gitster.g>
In-Reply-To: <BE9D3BD5-EC19-4881-872D-CEB4039A382D@jrtc27.com>

Jessica Clarke <jrtc27@jrtc27.com> writes:

Show 18 quoted lines

>> I actually wonder if it results in code that is much easier to
>> follow if we did:
>> 
>> * Introduce an "enum apply_symlink_treatment" that has
>>   APPLY_SYMLINK_GOES_AWAY and APPLY_SYMLINK_IN_RESULT as its
>>   possible values;
>> 
>> * Make register_symlink_changes() and check_symlink_changes()
>>   work with "enum apply_symlink_treatment";
>> 
>> * (optional) stop using string_list() to store the symlink_changes;
>>   use strintmap and use strintmap_set() and strintmap_get() to
>>   access its entries, so that the ugly implementation detail
>>   (i.e. "the container type we use only has a (void *) field to
>>   store caller-supplied data, so we cast an integer and a pointer
>>   back and forth") can be safely hidden.
>
> Those would be better if you want a less-minimal change.

The first two at least would make an understandable change, as opposed to the code as posted, which is totally opaque why we need to take size_t and cast it to uintptr_t in the first place.

I have no confidence in myself that I would not accept a future patch that reverts the change made by this patch happily. I usually try to be careful to go back to the originating commit by running "blame" on the preimage and may find your commit, but that's only "usually". If I were to work only with the file contents after applying this patch, because no clue ...

	ent->util = (void *)((uintptr_t)what | ((uintptr_t)ent->util));

... on this line of code hints why we must be called with size_t and have to cast it here, instead of working with uintptr_t throughout, I am reasonably sure I'd happily take such a patch and break your "fix" here.

If we make the code pass around the enum, have a temporary variable of the same enum type to compute the next value we stuff in ent->util, and make an assignment of that enum value to ent->util, it is much less likely that I'd blindly accept a patch to take us back to deal with uintptr_t directly.