Skip to content

pr-2297/thomasbachem/config-null-value-segfaults-v1

tagged this 07 Nov 08:42
A setting given without a value, as in "git -c foo.bar" or a bare "bar"
under "[foo]", is passed as NULL to the code that reads it. In these places
git does not check for that and segfaults:

 * rebase.missingCommitsCheck in "git rebase -i"
 * core.alternateRefsCommand and core.alternateRefsPrefixes in "git fetch",
   "git receive-pack" and "git rev-list --alternate-refs", in a repository
   with an alternate
 * the color slots of "git add -p", "git add -i" and the other interactive
   patch modes
 * fetch.bundleCreationToken in "git fetch" and "git clone --bundle-uri",
   with a creationToken bundle list
 * grep.patternType in "git grep", "git log" and "git show"
 * remote.*.tagOpt in "git fetch", "git remote" and "git status"
 * color.blame.repeatedLines and color.blame.highlightRecent in "git blame"
 * branch.*.mergeOptions in "git merge" on that branch
 * remotes.* in "git fetch" and "git push" of that remote group
 * safe.bareRepository in any bare repository that git discovers
 * bitmapPseudoMerge.*.pattern when writing a bitmap, as in "git repack
   -adb"
 * scalar.repo in "scalar reconfigure --all"
 * remote.*.url in nearly every command, when an includeIf
   "hasconfig:remote.*.url:..." section exists
 * mode, heuristic, uri and creationToken in a bundle list, in "git clone
   --bundle-uri" and "git fetch"
 * any key at all in "git config list --type=color" and "--type=expiry-date"

These turned up in a check of every config callback and every caller of
repo_config_get_value(), and in runs of two dozen common commands with each
key that "git help --config" lists set without a value. There may be more in
code that neither method reached.

Most patches reject the missing value with config_error_nonbool(), as
ba176db511 (config: handle NULL value when parsing non-bools, 2023-12-07)
did for other settings. Three accept it without an error: a missing
fetch.bundleCreationToken counts as 0 (patch 4), a remote URL without a
value matches no includeIf condition (patch 13), and "git config list"
leaves the key out (patch 15). The crash fixed in patch 14 can also come
from a server, through a bundle list it advertises to clients that use
bundle URIs. It is still a plain NULL dereference. Only patch 15 fixes a
regression, which came in with 2.54.0.

remote.*.followRemoteHEAD without a value crashes the same commands. This
series leaves it alone, as ch/remote-defer-follow-remote-head-validation in
'seen' already makes it safe, checking the value only where fetch uses it.

The series is based on master and merges cleanly there. In 'next' and
'seen', t5618 conflicts with iw/connected-deepen-fetch-no-alternate-refs,
and in 'seen' also with kk/quarantine-alternate-refs, as each adds tests at
the end of the file. Keeping both sides resolves it.

Thomas Bachem (15):
  rebase -i: handle NULL value for rebase.missingCommitsCheck
  odb: handle NULL values for core.alternateRefs* config
  add-patch: handle NULL value for color slots
  bundle-uri: handle NULL value for fetch.bundleCreationToken
  grep: handle NULL value for grep.patternType
  remote: handle NULL value for remote.<name>.tagOpt
  blame: handle NULL values for color.blame.* config
  merge: handle NULL value for branch.<name>.mergeOptions
  remote: handle NULL value for remotes.<group>
  setup: handle NULL value for safe.bareRepository
  pseudo-merge: handle NULL value for bitmapPseudoMerge.<name>.pattern
  scalar: handle NULL value for scalar.repo
  config: handle NULL value for remote URLs in hasconfig includes
  bundle-uri: handle NULL values in bundle lists
  config: handle NULL values when listing colors and expiry dates

 add-patch.c                     |  2 +-
 builtin/blame.c                 |  4 ++++
 builtin/config.c                |  4 ++--
 builtin/merge.c                 |  2 ++
 bundle-uri.c                    | 12 +++++++++++-
 config.c                        |  3 ++-
 grep.c                          |  2 ++
 odb.c                           |  4 ++--
 pseudo-merge.c                  |  4 ++++
 rebase-interactive.c            |  2 +-
 remote.c                        |  7 ++++++-
 scalar.c                        |  5 ++++-
 setup.c                         |  2 ++
 t/t0035-safe-bare-repository.sh |  6 ++++++
 t/t1300-config.sh               | 32 ++++++++++++++++++++++++++++++++
 t/t3404-rebase-interactive.sh   |  7 +++++++
 t/t3701-add-interactive.sh      | 12 ++++++++++++
 t/t5333-pseudo-merge-bitmaps.sh |  5 +++++
 t/t5505-remote.sh               |  5 +++++
 t/t5506-remote-groups.sh        |  5 +++++
 t/t5558-clone-bundle-uri.sh     | 20 ++++++++++++++++++++
 t/t5618-alternate-refs.sh       |  9 +++++++++
 t/t5750-bundle-uri-parse.sh     | 16 ++++++++++++++++
 t/t7600-merge.sh                |  6 ++++++
 t/t7810-grep.sh                 |  5 +++++
 t/t8012-blame-colors.sh         |  7 +++++++
 t/t9210-scalar.sh               |  5 +++++
 27 files changed, 183 insertions(+), 10 deletions(-)

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd

Submitted-As: https://lore.kernel.org/git/pull.2297.git.1794040967.gitgitgadget@gmail.com
Assets 2
Loading