[PATCH v5 00/27] Compile with `-Wwrite-strings`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jun 6, 2024, 10:27 UTC
- Message-ID
- <cover.1717667854.git.ps@pks.im>
- In-Reply-To
- <cover.1716983704.git.ps@pks.im>
Hi,
this is another version of my patch series that prepares our code base to compile with `-Wwrite-strings`. The effect of that warning is to turn string constants from type `char []` to `const char []` so that we can detect more readily when something may accidentally try to write to or free a constant.
Changes compared to v5:
- Plug a memory leak in `pretend_object_file()`.
- Drop `rebase_options_init()` in favor of `REBASE_OPTIONS_INIT`.
Thanks!
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 | 27 +++++--- 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, 448 insertions(+), 368 deletions(-)
Range-diff against v4:
1: e01fde88fe = 1: e01fde88fe global: improve const correctness when assigning string constants
2: 92cb0b28c6 = 2: 92cb0b28c6 global: convert intentionally-leaking config strings to consts
3: 379145478c = 3: 379145478c refs/reftable: stop micro-optimizing refname allocations on copy
4: d0a2a2f6c5 = 4: d0a2a2f6c5 reftable: cast away constness when assigning constants to records
5: ead27d3d97 = 5: ead27d3d97 refspec: remove global tag refspec structure
6: 7cb5df9182 = 6: 7cb5df9182 builtin/remote: cast away constness in `get_head_names()`
7: 6e631a9ea4 = 7: 6e631a9ea4 diff: cast string constant in `fill_textconv()`
8: ac164651a3 = 8: ac164651a3 line-log: stop assigning string constant to file parent buffer
9: b717af02f0 = 9: b717af02f0 line-log: always allocate the output prefix
10: b46dd3210d = 10: b46dd3210d entry: refactor how we remove items for delayed checkouts
11: 030dbd0288 = 11: 030dbd0288 ident: add casts for fallback name and GECOS
12: ecca8e973d ! 12: 5cd014c22c object-file: mark cached object buffers as const
@@ object-file.c: int pretend_object_file(void *buf, unsigned long len, enum object
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) ||
-@@ object-file.c: int pretend_object_file(void *buf, unsigned long len, enum object_type type,
+- 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;
13: 62f0e47f94 = 13: 69d904ddce object-file: make `buf` parameter of `index_mem()` a constant
14: e057ead2b4 = 14: ed8f07aa59 pretty: add casts for decoration option pointers
15: 06b6120d26 = 15: 5953ae1dac compat/win32: fix const-correctness with string constants
16: a8ef39d73d = 16: c80f6eff8c http: do not assign string constant to non-const field
17: 9d596a07c5 = 17: 3afd012a88 parse-options: cast long name for OPTION_ALIAS
18: 4019b532f9 = 18: 527755b648 send-pack: always allocate receive status
19: f2f1ada143 = 19: 4598592d2f remote-curl: avoid assigning string constant to non-const variable
20: 27660b908c = 20: 38fcea2845 revision: always store allocated strings in output encoding
21: ef43c1b18f = 21: f990bbeb85 mailmap: always store allocated strings in mailmap blob
22: 0a69ce4b44 = 22: fff2379832 imap-send: drop global `imap_server_conf` variable
23: 9ccafd286b = 23: 9ab84e459a imap-send: fix leaking memory in `imap_server_conf`
24: e19457d20c ! 24: 81c69da2e8 builtin/rebase: do not assign default backend to non-constant field
@@ Commit message
The `struct rebase_options::default_backend` field is a non-constant
string, but is being assigned a constant via `REBASE_OPTIONS_INIT`.
- Refactor the code to initialize and release options via two functions
- `rebase_options_init()` and `rebase_options_release()`. Like this, we
- can easily adapt the former funnction to use `xstrdup()` on the default
- value without hiding it away in a macro.
+ Fix this by using `xstrdup()` to assign the variable and introduce a new
+ function `rebase_options_release()` that releases memory held by the
+ structure, including the newly-allocated variable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## builtin/rebase.c ##
@@ builtin/rebase.c: struct rebase_options {
- int config_update_refs;
- };
-
--#define REBASE_OPTIONS_INIT { \
-- .type = REBASE_UNSPECIFIED, \
-- .empty = EMPTY_UNSPECIFIED, \
-- .keep_empty = 1, \
+ .type = REBASE_UNSPECIFIED, \
+ .empty = EMPTY_UNSPECIFIED, \
+ .keep_empty = 1, \
- .default_backend = "merge", \
-- .flags = REBASE_NO_QUIET, \
-- .git_am_opts = STRVEC_INIT, \
-- .exec = STRING_LIST_INIT_NODUP, \
-- .git_format_patch_opt = STRBUF_INIT, \
-- .fork_point = -1, \
-- .reapply_cherry_picks = -1, \
-- .allow_empty_message = 1, \
-- .autosquash = -1, \
-- .rebase_merges = -1, \
-- .config_rebase_merges = -1, \
-- .update_refs = -1, \
-- .config_update_refs = -1, \
-- .strategy_opts = STRING_LIST_INIT_NODUP,\
-- }
-+static void rebase_options_init(struct rebase_options *opts)
-+{
-+ memset(opts, 0, sizeof(*opts));
-+ opts->type = REBASE_UNSPECIFIED;
-+ opts->empty = EMPTY_UNSPECIFIED;
-+ opts->default_backend = xstrdup("merge");
-+ opts->keep_empty = 1;
-+ opts->flags = REBASE_NO_QUIET;
-+ strvec_init(&opts->git_am_opts);
-+ string_list_init_nodup(&opts->exec);
-+ strbuf_init(&opts->git_format_patch_opt, 0);
-+ opts->fork_point = -1;
-+ opts->reapply_cherry_picks = -1;
-+ opts->allow_empty_message = 1;
-+ opts->autosquash = -1;
-+ opts->rebase_merges = -1;
-+ opts->config_rebase_merges = -1;
-+ opts->update_refs = -1;
-+ opts->config_update_refs = -1;
-+ string_list_init_nodup(&opts->strategy_opts);
-+}
-+
++ .default_backend = xstrdup("merge"), \
+ .flags = REBASE_NO_QUIET, \
+ .git_am_opts = STRVEC_INIT, \
+ .exec = STRING_LIST_INIT_NODUP, \
+@@ builtin/rebase.c: struct rebase_options {
+ .strategy_opts = STRING_LIST_INIT_NODUP,\
+ }
+
+static void rebase_options_release(struct rebase_options *opts)
+{
+ free(opts->default_backend);
@@ builtin/rebase.c: struct rebase_options {
+ string_list_clear(&opts->strategy_opts, 0);
+ strbuf_release(&opts->git_format_patch_opt);
+}
-
++
static struct replay_opts get_replay_opts(const struct rebase_options *opts)
{
+ struct replay_opts replay = REPLAY_OPTS_INIT;
@@ builtin/rebase.c: static int rebase_config(const char *var, const char *value,
}
@@ builtin/rebase.c: static int rebase_config(const char *var, const char *value,
return git_config_string(&opts->default_backend, var, value);
}
-@@ builtin/rebase.c: static int check_exec_cmd(const char *cmd)
-
- int cmd_rebase(int argc, const char **argv, const char *prefix)
- {
-- struct rebase_options options = REBASE_OPTIONS_INIT;
-+ struct rebase_options options;
- const char *branch_name;
- int ret, flags, total_argc, in_progress = 0;
- int keep_base = 0;
-@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- };
- int i;
-
-+ rebase_options_init(&options);
-+
- if (argc == 2 && !strcmp(argv[1], "-h"))
- usage_with_options(builtin_rebase_usage,
- builtin_rebase_options);
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
cleanup:
strbuf_release(&buf);
25: f548241960 ! 25: 6819bf6116 builtin/rebase: always store allocated string in `options.strategy`
@@ Commit message
## builtin/rebase.c ##
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
{
- struct rebase_options options;
+ struct rebase_options options = REBASE_OPTIONS_INIT;
const char *branch_name;
+ const char *strategy_opt = NULL;
int ret, flags, total_argc, in_progress = 0;
26: 78ac075644 = 26: a1d2149429 builtin/merge: always store allocated strings in `pull_twohead`
27: 0cd4ce07d8 = 27: c714b67199 config.mak.dev: enable `-Wwrite-strings` warning-- 2.45.2.409.g7b0defb391.dirty