[PATCH 00/19] Compile with `-Wwrite-strings`

Hi,

there were some recent discussions about compiler warnings and how to stay on top of breaking changes in compilers in general [1] and about string constants in particular [2]. This made me look into what kind of warnings we should reasonably enable, which led me to the following list of warnings that may be sensible:

  - `-Wformat-nonliteral` to warn about non-constant strings being
    passed as format string.

  - `-Wwrite-strings` to warn about string constants being assigned to a
    non-constant variable.

  - `-Wredundant-decls` to warn about redundant declarations.

  - `-Wconversion` to warn about implicit integer casts when they may
    alter the value.

This patch series adapts our code to compile with `-Wwrite-strings`. This option will change the type of string constants from `char []` to `const char []` such that it is now invalid to assign it to non-const variables without a cast. The intent is to avoid undefined behaviour when accedintally writing to such strings and to avoid free'ing such a variable.

There are quite some cases where we mishandle this. Oftentimes we just didn't bother to free any memory at all, which made it a non-issue in the first place. Other times we had some special logic that prevents writing or freeing such strings. But in most cases it was an accident waiting to happen.

Even though the changes are quite invasive, I think that this is a step into the right direction. Many of the constructs feel quite fragile, and most of those get fixed in this series. Some others I just paper over, for example when assigning to structures with global lifetime where we know that they are never released at all.

I also have a patch series cooking for `-Wredundant-decls`. But while that warning detects some redundant declarations indeed, it creates a problem with `extern char **environ`. There is no header for it and programs are asked to declare it by themselves. But of course, some libc implementations disagree and declare it. I haven't found a nice way to work around this issue, but may send the patches that drop the redundant declarations nonetheless.

The other two warnings I haven't yet looked into.

I ran some test jobs on both GitHub [3] and GitLab [4] to verify that the result is sane.

Thanks!

Patrick

[1]: <xmqqbk5bim2n.fsf@gitster.g> [2]: <20240525043347.GA1895047@coredump.intra.peff.net> [3]: https://github.com/git/git/pull/1730 [4]: https://gitlab.com/gitlab-org/git/-/pipelines/1310156791

Patrick Steinhardt (19):
  global: improve const correctness when assigning string constants
  global: assign non-const strings as required
  global: convert intentionally-leaking config strings to consts
  compat/win32: fix const-correctness with string constants
  reftable: improve const correctness when assigning string constants
  refspec: remove global tag refspec structure
  http: do not assign string constant to non-const field
  line-log: always allocate the output prefix
  object-file: make `buf` parameter of `index_mem()` a constant
  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: adapt code to not assign string constants to non-const
  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             |  16 +++--
 builtin/receive-pack.c       |   4 +-
 builtin/remote.c             |   3 +-
 builtin/revert.c             |   2 +-
 builtin/send-pack.c          |   2 +
 compat/basename.c            |  15 +++-
 compat/mingw.c               |  25 +++----
 compat/regex/regcomp.c       |   2 +-
 compat/winansi.c             |   2 +-
 config.mak.dev               |   1 +
 diff.c                       |   7 +-
 diffcore-rename.c            |   6 +-
 entry.c                      |   7 +-
 fmt-merge-msg.c              |   2 +-
 fsck.c                       |   2 +-
 fsck.h                       |   2 +-
 gpg-interface.c              |   6 +-
 http-backend.c               |   2 +-
 http.c                       |   5 +-
 ident.c                      |   9 ++-
 imap-send.c                  | 133 ++++++++++++++++++++---------------
 line-log.c                   |  21 +++---
 mailmap.c                    |   2 +-
 merge-ll.c                   |  11 ++-
 object-file.c                |  17 ++---
 parse-options.h              |   2 +-
 pretty.c                     |   7 +-
 refs.c                       |   2 +-
 refs.h                       |   2 +-
 refs/reftable-backend.c      |   5 +-
 refspec.c                    |  13 ----
 refspec.h                    |   1 -
 reftable/basics_test.c       |   4 +-
 reftable/block_test.c        |   2 +-
 reftable/merged_test.c       |  45 ++++++------
 reftable/readwrite_test.c    |  47 +++++++------
 reftable/record.c            |   6 +-
 reftable/stack_test.c        |  65 ++++++++---------
 remote-curl.c                |  58 ++++++++-------
 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 +-
 65 files changed, 410 insertions(+), 340 deletions(-)