Fix segfaults on settings given without a value
Get series with b4
b4 am [email protected]Versions newest first · linked by author and title
- v2 Latest15 patches 5 repliesIn review
- v115 patches 11 repliesSuperseded
Changes in v2 from the cover letter
* Patch 1 no longer rejects a rebase.missingCommitsCheck without a value. It warns and falls back to "ignore", as for a value that git does not recognize (Phillip, Junio).
* Rebased onto master, where iw/connected-deepen-fetch-no-alternate-refs now adds tests at the end of t5618, as patch 2 does.
Patches in v2 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/15 | rebase -i: handle NULL value for rebase.missingCommitsCheckrebase-interactive.c t/ | +20 | 1 reply | |
| 2/15 | odb: handle NULL values for core.alternateRefs* configodb.c t/ | +11 | 0 replies | |
| 3/15 | add-patch: handle NULL value for color slotsadd-patch.c t/ | +13 | 0 replies | |
| 4/15 | bundle-uri: handle NULL value for fetch.bundleCreationTokenbundle-uri.c t/ | +22 | 0 replies | |
| 5/15 | grep: handle NULL value for grep.patternTypegrep.c t/ | +7 | 0 replies | |
| 6/15 | remote: handle NULL value for remote.<name>.tagOptremote.c t/ | +7 | 0 replies | |
| 7/15 | blame: handle NULL values for color.blame.* configbuiltin/ t/ | +11 | 0 replies | |
| 8/15 | merge: handle NULL value for branch.<name>.mergeOptionsbuiltin/ t/ | +8 | 0 replies | |
| 9/15 | remote: handle NULL value for remotes.<group>remote.c t/ | +9 | 0 replies | |
| 10/15 | setup: handle NULL value for safe.bareRepositorysetup.c t/ | +8 | 0 replies | |
| 11/15 | pseudo-merge: handle NULL value for bitmapPseudoMerge.<name>.patternpseudo-merge.c t/ | +9 | 0 replies | |
| 12/15 | scalar: handle NULL value for scalar.reposcalar.c t/ | +9 | 0 replies | |
| 13/15 | config: handle NULL value for remote URLs in hasconfig includesconfig.c t/ | +24 | 0 replies | |
| 14/15 | bundle-uri: handle NULL values in bundle listsbundle-uri.c t/ | +25 | 2 replies | |
| 15/15 | config: handle NULL values when listing colors and expiry datesbuiltin/ t/ | +12 | 0 replies |
Reviews trailers per patch
None of the 15 patches has a Reviewed-by or Acked-by.
| Patch | Phillip Wood |
|---|---|
| 1/15 | Helped-by |
| 2/15 | |
| 3/15 | |
| 4/15 | |
| 5/15 | |
| 6/15 | |
| 7/15 | |
| 8/15 | |
| 9/15 | |
| 10/15 | |
| 11/15 | |
| 12/15 | |
| 13/15 | |
| 14/15 | |
| 15/15 |
Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.
Cover letter [PATCH v2 00/15] Fix segfaults on settings given without a value
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"
Changes since v1:
* Patch 1 no longer rejects a rebase.missingCommitsCheck without a value. It warns and falls back to "ignore", as for a value that git does not recognize (Phillip, Junio).
* Rebased onto master, where iw/connected-deepen-fetch-no-alternate-refs now adds tests at the end of t5618, as patch 2 does.
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. Four accept it without an error: a missing rebase.missingCommitsCheck warns and falls back to "ignore" like a value that git does not recognize (patch 1), 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 now based on 3c60ef1e38 (The 17th batch, 2026-11-08) and merges cleanly into 'next' and 'seen' too.
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 | 10 ++++++++-- 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 | 12 ++++++++++++ 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, 195 insertions(+), 11 deletions(-)
base-commit: 3c60ef1e38ee80337025a1b908327f7879386329
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2297%2Fthomasbachem%2Fconfig-null-value-segfaults-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2297/thomasbachem/config-null-value-segfaults-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2297
Range-diff vs v1:
1: a9a3f4b705 ! 1: 47224a9893 rebase -i: handle NULL value for rebase.missingCommitsCheck
@@ Commit message
segfault as it writes the todo list: get_missing_commit_check_level()
passes the NULL value to strcasecmp().
- The setting takes "ignore", "warn" or "error", not a bool. Read it
- with repo_config_get_string_tmp(), which rejects a missing value with
- config_error_nonbool(), as ba176db511 (config: handle NULL value when
- parsing non-bools, 2023-12-07) did for other settings.
+ The setting takes "ignore", "warn" or "error". An unrecognized value,
+ perhaps one meant for a newer version of Git, gets a warning and is
+ treated as "ignore". Treat a missing value the same way.
+ Helped-by: Phillip Wood <[email protected]>
Assisted-by: Claude Fable 5.1
+ Assisted-by: Claude Opus 5.5
Signed-off-by: Thomas Bachem <[email protected]>
## rebase-interactive.c ##
@@ rebase-interactive.c: static enum missing_commit_check_level get_missing_commit_
const char *value;
- if (repo_config_get_value(the_repository, "rebase.missingcommitscheck", &value) ||
-+ if (repo_config_get_string_tmp(the_repository, "rebase.missingcommitscheck", &value) ||
- !strcasecmp("ignore", value))
+- !strcasecmp("ignore", value))
++ if (repo_config_get_value(the_repository, "rebase.missingcommitscheck", &value))
++ return MISSING_COMMIT_CHECK_IGNORE;
++ if (!value) {
++ warning(_("missing value for option "
++ "rebase.missingCommitsCheck. Ignoring."));
++ return MISSING_COMMIT_CHECK_IGNORE;
++ }
++ if (!strcasecmp("ignore", value))
return MISSING_COMMIT_CHECK_IGNORE;
if (!strcasecmp("warn", value))
+ return MISSING_COMMIT_CHECK_WARN;
## t/t3404-rebase-interactive.sh ##
@@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
'
-+test_expect_success 'rebase -i rejects rebase.missingCommitsCheck without a value' '
++test_expect_success 'rebase -i warns about and ignores rebase.missingCommitsCheck without a value' '
+ rebase_setup_and_clean missing-commit &&
-+ test_when_finished "test_might_fail git rebase --abort" &&
-+ test_must_fail git -c rebase.missingCommitsCheck rebase -i --root 2>err &&
-+ test_grep "missing value for .rebase.missingcommitscheck." err
++ (
++ set_fake_editor &&
++ FAKE_LINES="1 2 3 4" \
++ git -c rebase.missingCommitsCheck rebase -i --root 2>actual
++ ) &&
++ test D = $(git cat-file commit HEAD | sed -ne \$p) &&
++ test_grep "missing value for option rebase.missingCommitsCheck" actual &&
++ test_grep ! "commits may have been dropped" actual
+'
+
test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' '
2: 74ab5bfc62 ! 2: 9170c4162f odb: handle NULL values for core.alternateRefs* config
@@ odb.c: static void fill_alternate_refs_command(struct repository *repo,
}
## t/t5618-alternate-refs.sh ##
-@@ t/t5618-alternate-refs.sh: test_expect_success 'log --source shows .alternate marker' '
+@@ t/t5618-alternate-refs.sh: test_expect_success 'refs of a shallow alternate are ignored' '
test_cmp expect actual
'3: 1b5f243294 = 3: d3fd6290e0 add-patch: handle NULL value for color slots 4: 4eb7085084 = 4: a822cbb844 bundle-uri: handle NULL value for fetch.bundleCreationToken 5: 88df66ab7e = 5: 6c2a671a92 grep: handle NULL value for grep.patternType 6: e95249fb51 = 6: d4b49d1363 remote: handle NULL value for remote.<name>.tagOpt 7: 260b90a4a2 = 7: 25843b43bf blame: handle NULL values for color.blame.* config 8: e10f1fc0ba = 8: d8681a8560 merge: handle NULL value for branch.<name>.mergeOptions 9: 0faf4e884f = 9: 51f8eba073 remote: handle NULL value for remotes.<group> 10: 0991a1b80c = 10: a5b6436943 setup: handle NULL value for safe.bareRepository 11: 3f4cbf4494 = 11: 1533ef9cc6 pseudo-merge: handle NULL value for bitmapPseudoMerge.<name>.pattern 12: b4bcaf19ec = 12: 6219e9fd7f scalar: handle NULL value for scalar.repo 13: 5c12b13d90 = 13: 080106d5ea config: handle NULL value for remote URLs in hasconfig includes 14: ce4c190ea2 = 14: fb9e63541b bundle-uri: handle NULL values in bundle lists 15: 7ac4e3a3d9 = 15: 12f841e5ff config: handle NULL values when listing colors and expiry dates
Signature · 1 line
gitgitgadget