[PATCH v5 00/27] Compile with `-Wwrite-strings`

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 13113 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]