| Message ID | cover.1716983704.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Compile with `-Wwrite-strings` | expand |
On Wed, May 29, 2024 at 02:44:01PM +0200, Patrick Steinhardt wrote: > 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! I forgot to say that this is based on top of 3a57aa566a (The eighth batch, 2024-05-28) with ps/leakfixes merged into it at ebdbefa4fe (builtin/mv: fix leaks for submodule gitfile paths, 2024-05-27). Patrick
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(-)