Re: [PATCH v4 12/27] object-file: mark cached object buffers as const
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 6, 2024, 06:02 UTC
- Message-ID
- <xmqqr0dapq1s.fsf@gitster.g>
- In-Reply-To
- <ecca8e973df77cfc8233ab63bf7d1f6fa83031a3.1717504517.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 14 quoted lines
> The buffers of cached objects are never modified, but are still stored > as a non-constant pointer. This will cause a compiler warning once we > enable the `-Wwrite-strings` compiler warning as we assign an empty > constant string when initializing the static `empty_tree` cached object. > > Convert the field to be constant. This requires us to shuffle around > the code a bit because we memcpy(3P) into the allocated buffer in > `pretend_object_file()`. This is easily fixed though by allocating the > buffer into a temporary variable first. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > object-file.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-)
I haven't dug into the exact details, but I noticed that t6130, t7010, and t8002 started breaking linux-leaks/linux-reftable-leaks CI jobs for 'seen'. 'seen' excluding ps/no-writable-strings seems to pass the tests, and bisection indicates that 'seen' excluding ps/no-writable-strings after this step [12/27] still passes, but once this step is included, the leak tests break.
Show 24 quoted lines
> diff --git a/object-file.c b/object-file.c
> index 610b1f465c..3afe9fce06 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -277,7 +277,7 @@ int hash_algo_by_length(int len)
> static struct cached_object {
> struct object_id oid;
> enum object_type type;
> - void *buf;
> + const void *buf;
> unsigned long size;
> } *cached_objects;
> static int cached_object_nr, cached_object_alloc;
> @@ -1778,6 +1778,10 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,
> struct object_id *oid)
> {
> struct cached_object *co;
> + char *co_buf;
> +
> + co_buf = xmalloc(len);
> + memcpy(co_buf, buf, len);
>
> hash_object_file(the_hash_algo, buf, len, type, oid);
> if (repo_has_object_file_with_flags(the_repository, oid, OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT) ||There is an early return around here. Perhaps we are leaking co_buf when we hit it?
Show 10 quoted lines
> @@ -1787,8 +1791,7 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type, > co = &cached_objects[cached_object_nr++]; > co->size = len; > co->type = type; > - co->buf = xmalloc(len); > - memcpy(co->buf, buf, len); > + co->buf = co_buf; > oidcpy(&co->oid, oid); > return 0; > }