mbox series

[v6,00/27] Compile with `-Wwrite-strings`

Message ID cover.1717740365.git.ps@pks.im (mailing list archive)
Headers show
Series Compile with `-Wwrite-strings` | expand

Message

Patrick Steinhardt June 7, 2024, 6:37 a.m. UTC 
Hi,

this is v6 of my patch series that starts to compile our codebase with
`-Wwrite-strings`. This warning turns the type of string constants from
`char []` to `const char []` and should thus help us to detect cases
where we may accidentally write to or free such a string constant.

The only change is in patch 12, where we now allocate a buffer later in
time to avoid a useless allocation and the need for a call to free.

Patrick

Patrick Steinhardt (27):
  global: improve const correctness when assigning string constants
  global: convert intentionally-leaking config strings to consts
  refs/reftable: stop micro-optimizing refname allocations on copy
  reftable: cast away constness when assigning constants to records
  refspec: remove global tag refspec structure
  builtin/remote: cast away constness in `get_head_names()`
  diff: cast string constant in `fill_textconv()`
  line-log: stop assigning string constant to file parent buffer
  line-log: always allocate the output prefix
  entry: refactor how we remove items for delayed checkouts
  ident: add casts for fallback name and GECOS
  object-file: mark cached object buffers as const
  object-file: make `buf` parameter of `index_mem()` a constant
  pretty: add casts for decoration option pointers
  compat/win32: fix const-correctness with string constants
  http: do not assign string constant to non-const field
  parse-options: cast long name for OPTION_ALIAS
  send-pack: always allocate receive status
  remote-curl: avoid assigning string constant to non-const variable
  revision: always store allocated strings in output encoding
  mailmap: always store allocated strings in mailmap blob
  imap-send: drop global `imap_server_conf` variable
  imap-send: fix leaking memory in `imap_server_conf`
  builtin/rebase: do not assign default backend to non-constant field
  builtin/rebase: always store allocated string in `options.strategy`
  builtin/merge: always store allocated strings in `pull_twohead`
  config.mak.dev: enable `-Wwrite-strings` warning

 builtin/bisect.c             |   3 +-
 builtin/blame.c              |   2 +-
 builtin/bugreport.c          |   2 +-
 builtin/check-ignore.c       |   4 +-
 builtin/clone.c              |  14 ++--
 builtin/commit.c             |   6 +-
 builtin/diagnose.c           |   2 +-
 builtin/fetch.c              |  11 ++-
 builtin/log.c                |   2 +-
 builtin/mailsplit.c          |   4 +-
 builtin/merge.c              |  18 +++--
 builtin/pull.c               |  52 +++++++-------
 builtin/rebase.c             |  39 ++++++-----
 builtin/receive-pack.c       |   4 +-
 builtin/remote.c             |  12 ++--
 builtin/revert.c             |   2 +-
 builtin/send-pack.c          |   2 +
 compat/basename.c            |  16 ++++-
 compat/mingw.c               |  28 ++++----
 compat/regex/regcomp.c       |   2 +-
 compat/winansi.c             |   2 +-
 config.mak.dev               |   1 +
 diff.c                       |   6 +-
 diffcore-rename.c            |   6 +-
 entry.c                      |  14 ++--
 fmt-merge-msg.c              |   2 +-
 fsck.c                       |   2 +-
 fsck.h                       |   2 +-
 gpg-interface.c              |   6 +-
 http-backend.c               |   2 +-
 http.c                       |   5 +-
 ident.c                      |   4 +-
 imap-send.c                  | 130 ++++++++++++++++++++---------------
 line-log.c                   |  22 +++---
 mailmap.c                    |   2 +-
 merge-ll.c                   |  11 ++-
 object-file.c                |  22 +++---
 parse-options.h              |   2 +-
 pretty.c                     |   6 +-
 refs.c                       |   2 +-
 refs.h                       |   2 +-
 refs/reftable-backend.c      |  28 ++++----
 refspec.c                    |  13 ----
 refspec.h                    |   1 -
 reftable/basics.c            |  15 ++--
 reftable/basics.h            |   4 +-
 reftable/basics_test.c       |   4 +-
 reftable/block_test.c        |   2 +-
 reftable/merged_test.c       |  44 ++++++------
 reftable/readwrite_test.c    |  32 ++++-----
 reftable/record.c            |   6 +-
 reftable/stack.c             |  10 +--
 reftable/stack_test.c        |  56 +++++++--------
 remote-curl.c                |  53 +++++++-------
 revision.c                   |   3 +-
 run-command.c                |   2 +-
 send-pack.c                  |   2 +-
 t/helper/test-hashmap.c      |   3 +-
 t/helper/test-json-writer.c  |  10 +--
 t/helper/test-regex.c        |   4 +-
 t/helper/test-rot13-filter.c |   5 +-
 t/t3900-i18n-commit.sh       |   1 +
 t/t3901-i18n-patch.sh        |   1 +
 t/unit-tests/t-strbuf.c      |  10 +--
 trailer.c                    |   2 +-
 userdiff.c                   |  10 +--
 userdiff.h                   |  12 ++--
 wt-status.c                  |   2 +-
 68 files changed, 444 insertions(+), 367 deletions(-)

Range-diff against v5:
 1:  e01fde88fe =  1:  ba50d96081 global: improve const correctness when assigning string constants
 2:  92cb0b28c6 =  2:  4769bdb893 global: convert intentionally-leaking config strings to consts
 3:  379145478c =  3:  90613bd582 refs/reftable: stop micro-optimizing refname allocations on copy
 4:  d0a2a2f6c5 =  4:  b126783ba3 reftable: cast away constness when assigning constants to records
 5:  ead27d3d97 =  5:  d10566e0a9 refspec: remove global tag refspec structure
 6:  7cb5df9182 =  6:  e9b8f9eaef builtin/remote: cast away constness in `get_head_names()`
 7:  6e631a9ea4 =  7:  8ea984128d diff: cast string constant in `fill_textconv()`
 8:  ac164651a3 =  8:  777b93a89a line-log: stop assigning string constant to file parent buffer
 9:  b717af02f0 =  9:  527533540e line-log: always allocate the output prefix
10:  b46dd3210d = 10:  4d0b8dcbaf entry: refactor how we remove items for delayed checkouts
11:  030dbd0288 = 11:  0d36b6e5d7 ident: add casts for fallback name and GECOS
12:  5cd014c22c ! 12:  bfd632fea7 object-file: mark cached object buffers as const
    @@ object-file.c: int pretend_object_file(void *buf, unsigned long len, enum object
      {
      	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);
    +@@ object-file.c: 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 = xmalloc(len);
    ++	memcpy(co_buf, buf, len);
     +	co->buf = co_buf;
      	oidcpy(&co->oid, oid);
      	return 0;
13:  69d904ddce = 13:  4770470a84 object-file: make `buf` parameter of `index_mem()` a constant
14:  ed8f07aa59 = 14:  02e79d030b pretty: add casts for decoration option pointers
15:  5953ae1dac = 15:  57402de20a compat/win32: fix const-correctness with string constants
16:  c80f6eff8c = 16:  71c83468f1 http: do not assign string constant to non-const field
17:  3afd012a88 = 17:  f3e02df0ca parse-options: cast long name for OPTION_ALIAS
18:  527755b648 = 18:  a46708b898 send-pack: always allocate receive status
19:  4598592d2f = 19:  99c88897be remote-curl: avoid assigning string constant to non-const variable
20:  38fcea2845 = 20:  03556233d6 revision: always store allocated strings in output encoding
21:  f990bbeb85 = 21:  2bb96449e2 mailmap: always store allocated strings in mailmap blob
22:  fff2379832 = 22:  8059e013a1 imap-send: drop global `imap_server_conf` variable
23:  9ab84e459a = 23:  04c9cfd34d imap-send: fix leaking memory in `imap_server_conf`
24:  81c69da2e8 = 24:  b2b4a01a2e builtin/rebase: do not assign default backend to non-constant field
25:  6819bf6116 = 25:  09980d211b builtin/rebase: always store allocated string in `options.strategy`
26:  a1d2149429 = 26:  d5dc1453f2 builtin/merge: always store allocated strings in `pull_twohead`
27:  c714b67199 = 27:  2c051d1fe6 config.mak.dev: enable `-Wwrite-strings` warning

Comments

Junio C Hamano June 7, 2024, 5:34 p.m. UTC | #1 
Patrick Steinhardt <ps@pks.im> writes:

> The only change is in patch 12, where we now allocate a buffer later in
> time to avoid a useless allocation and the need for a call to free.

That makes sense (and is closer to the original anyway).

Will queue.  Thanks.