SZEDER Gábor <szeder....@gmail.com> writes:
> it appears that this patch (and its previous versions as well) is
> responsible for triggering occasional test failures in
> 't7814-grep-recurse-submodules.sh', more frequently, about once in
> every ten runs, on macOS on Travis CI, less frequently, about once in
> a couple of hundred runs on Linux on my machine.
I see that among Cc'ed are people who are more familiar with the
submodule code and where it wants to go. Thanks for a report and
analysis.
> The reason for the failure is memory corruption manifesting in various
> ways: segfault, malloc() or use after free() errors from libc, corrupt
> loose object, invalid ref, bogus output, etc.
>
> Applying the following patch makes t7814 fail almost every time,
> though sometimes that loop has to iterate over 1000 times until that
> 'git grep' finally fails... so good luck with debugging ;)
>
> diff --git a/t/t7814-grep-recurse-submodules.sh
> b/t/t7814-grep-recurse-submodules.sh
> index 7184113b9b..93ae2e8e7c 100755
> --- a/t/t7814-grep-recurse-submodules.sh
> +++ b/t/t7814-grep-recurse-submodules.sh
> @@ -337,6 +337,10 @@ test_expect_success 'grep --recurse-submodules should
> pass the pattern type alon
> test_must_fail git -c grep.patternType=fixed grep --recurse-submodules
> -e "(.|.)[\d]" &&
>
> # Basic
> + for i in $(seq 0 2000)
> + do
> + git grep --recurse-submodules 1 >/dev/null || return 1
> + done &&
> git grep -G --recurse-submodules -e "(.|.)[\d]" >actual &&
> cat >expect <<-\EOF &&
> a:(1|2)d(3|4)
>
> On first look I didn't notice anything that is obviously wrong in this
> patch and could be responsible for the memory corruption, but there is
> one thing I found strange, though:
>
>
> On Mon, Sep 17, 2018 at 04:09:40PM +0200, Antonio Ospite wrote:
>> When the .gitmodules file is not available in the working tree, try
>> using the content from the index and from the current branch.
>
> "from the index and from the current branch" of which repository?
>
>> This
>> covers the case when the file is part of the repository but for some
>> reason it is not checked out, for example because of a sparse checkout.
>>
>> This makes it possible to use at least the 'git submodule' commands
>> which *read* the gitmodules configuration file without fully populating
>> the working tree.
>>
>> Writing to .gitmodules will still require that the file is checked out,
>> so check for that before calling config_set_in_gitmodules_file_gently.
>>
>> Add a similar check also in git-submodule.sh::cmd_add() to anticipate
>> the eventual failure of the "git submodule add" command when .gitmodules
>> is not safely writeable; this prevents the command from leaving the
>> repository in a spurious state (e.g. the submodule repository was cloned
>> but .gitmodules was not updated because
>> config_set_in_gitmodules_file_gently failed).
>>
>> Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
>> from .gitmodules succeeds and that writing to it fails when the file is
>> not checked out.
>>
>> Signed-off-by: Antonio Ospite <a...@ao2.it>
>> ---
>
>> diff --git a/submodule-config.c b/submodule-config.c
>> index 61a555e920..bdb1d0e2c9 100644
>> --- a/submodule-config.c
>> +++ b/submodule-config.c
>
>> @@ -603,8 +604,21 @@ static void submodule_cache_check_init(struct
>> repository *repo)
>> static void config_from_gitmodules(config_fn_t fn, struct repository *repo,
>> void *data)
>> {
>> if (repo->worktree) {
>> - char *file = repo_worktree_path(repo, GITMODULES_FILE);
>> - git_config_from_file(fn, file, data);
>> + struct git_config_source config_source = { 0 };
>> + const struct config_options opts = { 0 };
>> + struct object_id oid;
>> + char *file;
>> +
>> + file = repo_worktree_path(repo, GITMODULES_FILE);
>> + if (file_exists(file))
>> + config_source.file = file;
>> + else if (get_oid(GITMODULES_INDEX, &oid) >= 0)
>> + config_source.blob = GITMODULES_INDEX;
>
> The repository used in t7814 contains nested submodules, which means
> that config_from_gitmodules() is invoked three times.
>
> Now, the first two of those calls look at the superproject and at
> 'submodule', and find the existing files '.../trash
> directory.t7814-grep-recurse-submodules/.gitmodules' and '.../trash
> directory.t7814-grep-recurse-submodules/submodule/.gitmodules',
> respectively. So far so good.
>
> The third call, however, looks at the nested submodule at
> 'submodule/sub', which doesn't contain a '.gitmodules' file. So this
> function goes on with the second condition and calls
> get_oid(GITMODULES_INDEX, &oid), which then appears to find the blob
> in the _superproject's_ index.
>
> I'm no expert on submodules, but my gut feeling says that this can't
> be right. But if it _is_ right, then I would say that the commit
> message should explain in detail, why it is right.
>
> Anyway, even if it is indeed wrong, I'm not sure whether this is the
> root cause of the memory corruption.
>
>
>> + else if (get_oid(GITMODULES_HEAD, &oid) >= 0)
>> + config_source.blob = GITMODULES_HEAD;
>> +
>> + config_with_options(fn, data, &config_source, &opts);
>> +
>> free(file);
>> }
>> }
