Hi,
On Tue, Sep 21, 2010 at 12:22 PM, Catalin Marinas
<catalin.marinas@xxxxxxx> wrote:
> Arnaud Lacombe <lacombar@xxxxxxxxx> wrote:
>> On Tue, Sep 21, 2010 at 8:32 AM, Catalin Marinas
>> <catalin.marinas@xxxxxxx> wrote:
>>> From: Catalin Marinas <catalin.marinas@xxxxxxx>
>>>
>>> Commit 246cf9c26b introduced the tracking of direct dependency to
>>> provide additional warning when they are not met. With some complex
>>> dependencies, the expr_copy() function called on such expressions may
>>> cause stack exhaustion. The patch removes the superfluous expr_copy()
>>> call since expr_transform handles symbol duplication already.
>>
>> I'm afraid this is just moving the problem from expr_copy() to
>> expr_transform() as the first thing the latter do is to recurse. on
>> the sub-expressions. I'm not sure that the expression causing the
>> stack exhaustion is even correct, or its reference is still valid.
>
> It may actually be just accessing freed symbols and the data being
> corrupted in ends up in a loop. You Kconfig example doesn't seem to have
> any dependency loops that would cause such problems.
>
exactly.
> Does the patch below make it any better (together with the previous
> one)?
>
just this one is enough. I added back the memory poisoning in
expr_free() and did not get any message of invalid expression.
Michal, should all the weak warning "can't copy/free expr" be replaced
by more aggressive abort() by dropping a few EXPR_CHECK() in expr.c ?
>
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index 7298806..e707aa2 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -107,7 +107,9 @@ static struct expr *menu_check_dep(struct expr *e)
> void menu_add_dep(struct expr *dep)
> {
> current_entry->dep = expr_alloc_and(current_entry->dep, menu_check_dep(dep));
> - current_entry->dir_dep = current_entry->dep;
> + if (current_entry->dir_dep)
> + expr_free(current_entry->dir_dep);
the test is not needed, expr_free() will return immediately if (e == NULL).
> + current_entry->dir_dep = expr_copy(current_entry->dep);
FYI, just this line makes the crash disappear, still I understand the
expr_free(). However, wouldn't there be a better place to initialize
`dir_dep' and avoid the need to use expr_free() ?
Thanks,
- Arnaud
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
