From patchwork Thu Jun 6 10:27:46 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 13688235 Received: from fhigh4-smtp.messagingengine.com (fhigh4-smtp.messagingengine.com [103.168.172.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2064E2E639 for ; Thu, 6 Jun 2024 10:27:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=103.168.172.155 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717669674; cv=none; b=myOp12Sf+gEpoj4/1W8joJ6vVzt91x0sY+G7HBKAHuSci9bH98xL2edWlbDMfk8my+8yQcPsCNMz9dqxrDdnb+MjFrLeoS7fmekvBPxvXH7FGbqS/BeNPFJ/iq6wkN3r0NK7OT/ujpCDQNFTJ6YDWHo5Keftu9KGSVR6xTazUEk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717669674; c=relaxed/simple; bh=abVeX9FAgb8LMTCNKOQMyhgSGbYwiqkW1j7zKgN6eqo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=RNjyRKK/mE8IWfIU9mf+1k5ItKwPRbCWQg1eTYtWjde7yQa2wdjvQMMCktL2gFOCN4BaTCsdA2a+LMknPuMhOubfYDfpZaMrDwQLowENWdqO5+IOq1Jj5ly/Vgeq/TrXYTnzbZRRPjX+MVPP7tTan8BBB56dIVAJmsUq8L9b6qE= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im; spf=pass smtp.mailfrom=pks.im; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b=GOz5MoQJ; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b=cRu4ePNM; arc=none smtp.client-ip=103.168.172.155 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=pks.im Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=pks.im Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=pks.im header.i=@pks.im header.b="GOz5MoQJ"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="cRu4ePNM" Received: from compute7.internal (compute7.nyi.internal [10.202.2.48]) by mailfhigh.nyi.internal (Postfix) with ESMTP id 2B96A11401AD; Thu, 6 Jun 2024 06:27:51 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Thu, 06 Jun 2024 06:27:51 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm1; t=1717669671; x=1717756071; bh=DvKOVuohD7 k2FtXkSJuWzInTX1LFq9R9u5lJslsT9As=; b=GOz5MoQJxaF+kVlwPc+A82TUxl EXiZWMbm3W5PY3UHneBrB6VV8BEgdGILFwnDh4jqCotSFJpFejCfOCbtJ3h4YbzM 6N4AYkLHGVhzjKaIr7mxiSHj5vvHAfRnuvBwHpntgr41vwJGzWfu69pmWPtKgU4r TyoCVRbQfzGYzkTU3+Mkk/T/SZ9ZaPY9ZeUtjJXAVFvDmsh9YGV4b1Xfmdy9AvLf paf7SBiB1sJJq5pkvnzmbJiwn0bNm4bZRiWExnubsd5BJT7U1+NIBHA4luZy1s5V XAcidmzZHXXpPsDS1BHff4z/gpXxZVT7WI44qUNnk6e4k4w1aFrPzfvPUZZQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; t=1717669671; x=1717756071; bh=DvKOVuohD7k2FtXkSJuWzInTX1LF q9R9u5lJslsT9As=; b=cRu4ePNMTEPsQ7fVibx2kst5w51cYgWh1XgDrEP1tsYl LbMJRfFM1snWvmTWbCoWqBiUCAXXkTQT/PTzdpCn0GBqQUfNJ5E1o3OBJD688n2W 931o0gJs5zQj6ZqOUVo9IhARNDmdSn3XYhoYW0RTn+vMq9fmfFQYl5TJo6GVfJTu AlLXKYRkINGcTg1lpEbSGV3vGfImLOSoEQTugnCnu4AUY4kRSkfN1f+NkcLX0urU LQ4mzX4Wd1B8G2PJoLFc7DiH/Vtxb1JOAxxxVhyNfhm0wIlrBH9v5XLe/8tDtp62 P9h2nVDWGoGkJx02cfGh2EWpmoEd1Yu+dySjic6Fyw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvledrvdelkedgvdekucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvfevuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhr ihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvg hrnheptdekfeegieeiudfgvdegfeffgfdtheegfeeiteffkeelteegffdvhfevjedujedt necuffhomhgrihhnpehmrghkrdguvghvnecuvehluhhsthgvrhfuihiivgeptdenucfrrg hrrghmpehmrghilhhfrhhomhepphhssehpkhhsrdhimh X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 6 Jun 2024 06:27:49 -0400 (EDT) Received: by localhost (OpenSMTPD) with ESMTPSA id 3120b308 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Thu, 6 Jun 2024 10:27:17 +0000 (UTC) Date: Thu, 6 Jun 2024 12:27:46 +0200 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Jeff King , Junio C Hamano , Eric Sunshine Subject: [PATCH v5 00/27] Compile with `-Wwrite-strings` Message-ID: References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: 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 ## 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