| Message ID | ecca8e973df77cfc8233ab63bf7d1f6fa83031a3.1717504517.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Compile with `-Wwrite-strings` | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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. > 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? > @@ -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; > }
Junio C Hamano <gitster@pobox.com> writes: >> + 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? Indeed, that seems to be the case. With the attached at the tip of the branch and rebuilding 'seen' seems to pass these 6130, 7010, 8002 tests with SANTIZE=leak. From f307bbf7bd317d90db29bd1589b49e84b9e37e88 Mon Sep 17 00:00:00 2001 From: Junio C Hamano <gitster@pobox.com> Date: Wed, 5 Jun 2024 23:03:34 -0700 Subject: [PATCH] fixup! object-file: mark cached object buffers as const --- object-file.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/object-file.c b/object-file.c index b5b5a59dc6..2d5bd3a211 100644 --- a/object-file.c +++ b/object-file.c @@ -1785,8 +1785,10 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type, 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) || - find_cached_object(oid)) + find_cached_object(oid)) { + free(co_buf); return 0; + } ALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc); co = &cached_objects[cached_object_nr++]; co->size = len;
On Wed, Jun 05, 2024 at 11:10:20PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > >> + 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? > > Indeed, that seems to be the case. With the attached at the tip of > the branch and rebuilding 'seen' seems to pass these 6130, 7010, 8002 > tests with SANTIZE=leak. Indeed, thanks. Rolled into my local version now. Patrick
Junio C Hamano <gitster@pobox.com> writes: > Indeed, that seems to be the case. With the attached at the tip of > the branch and rebuilding 'seen' seems to pass these 6130, 7010, 8002 > tests with SANTIZE=leak. > > From f307bbf7bd317d90db29bd1589b49e84b9e37e88 Mon Sep 17 00:00:00 2001 > From: Junio C Hamano <gitster@pobox.com> > Date: Wed, 5 Jun 2024 23:03:34 -0700 > Subject: [PATCH] fixup! object-file: mark cached object buffers as const > > --- > object-file.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/object-file.c b/object-file.c > index b5b5a59dc6..2d5bd3a211 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -1785,8 +1785,10 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type, > > 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) || > - find_cached_object(oid)) > + find_cached_object(oid)) { > + free(co_buf); > return 0; > + } > ALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc); > co = &cached_objects[cached_object_nr++]; > co->size = len; Wait. Why do we need to allocate co_buf that early in the first place? IOW, shouldn't the fixup be more like this? object-file.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git c/object-file.c w/object-file.c index b5b5a59dc6..0b58751f94 100644 --- c/object-file.c +++ w/object-file.c @@ -1780,9 +1780,6 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type, 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) || find_cached_object(oid)) @@ -1791,6 +1788,8 @@ 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;
On Thu, Jun 06, 2024 at 09:25:20AM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Indeed, that seems to be the case. With the attached at the tip of > > the branch and rebuilding 'seen' seems to pass these 6130, 7010, 8002 > > tests with SANTIZE=leak. > > > > From f307bbf7bd317d90db29bd1589b49e84b9e37e88 Mon Sep 17 00:00:00 2001 > > From: Junio C Hamano <gitster@pobox.com> > > Date: Wed, 5 Jun 2024 23:03:34 -0700 > > Subject: [PATCH] fixup! object-file: mark cached object buffers as const > > > > --- > > object-file.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/object-file.c b/object-file.c > > index b5b5a59dc6..2d5bd3a211 100644 > > --- a/object-file.c > > +++ b/object-file.c > > @@ -1785,8 +1785,10 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type, > > > > 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) || > > - find_cached_object(oid)) > > + find_cached_object(oid)) { > > + free(co_buf); > > return 0; > > + } > > ALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc); > > co = &cached_objects[cached_object_nr++]; > > co->size = len; > > Wait. Why do we need to allocate co_buf that early in the first > place? IOW, shouldn't the fixup be more like this? That is of course the much more elegant solution here. Will adapt. Patrick
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) || @@ -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; }
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(-)