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@xxxxxxxxxxxxxxxxxxxxxxx>
[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(-)
--
2.45.1.313.g3a57aa566a.dirty
Attachment:
signature.asc
Description: PGP signature
