| Message ID | 5cd014c22cb2fc7e34666aa6bd036d3cc4ce9039.1717667854.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Compile with `-Wwrite-strings` | expand |
Patrick Steinhardt <ps@pks.im> writes: > @@ -1778,17 +1778,22 @@ 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); I do not see why we need to do this so early. The copy is not used or buf gets modified by the call to hash_object_file(), so ... > 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; > co->type = type; > - co->buf = xmalloc(len); > - memcpy(co->buf, buf, len); > + co->buf = co_buf; ... wouldn't this be a better place to do the "copy to the heap memory pointed by a writable pointer and then point that piece of memory with a read-only pointer" pattern? > oidcpy(&co->oid, oid); > return 0; > }
diff --git a/object-file.c b/object-file.c index 610b1f465c..6309b24387 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,17 +1778,22 @@ 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) || - 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; 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 | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)