git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 1/2] negotiator/default: avoid stack overflow

From
Derrick Stolee <derrickstolee@github.com>
Date
Apr 26, 2023, 11:13 UTC
Message-ID
<bf45bfe7-4a29-38d6-b8d7-811581aec82b@github.com>
In-Reply-To
<935be72eb92cd2eda7aff43c8cc2306b78b2a146.1682473718.git.hanxin.hx@bytedance.com>
On 4/26/2023 12:05 AM, Han Xin wrote:
Show 7 quoted lines
> mark_common() in negotiator/default.c may overflow the stack due to
> recursive function calls. Avoid this by instead recursing using a
> heap-allocated data structure.
> 
> This is the same case as [1].
> 
> 1. 4654134976f (negotiator/skipping: avoid stack overflow, 2022-10-25)
We would typically write this inline, such as:
  This is the same case as 4654134976f (negotiator/skipping: avoid
  stack overflow, 2022-10-25)
Show 53 quoted lines
> -	if (commit != NULL && !(commit->object.flags & COMMON)) {
> -		struct object *o = (struct object *)commit;
> +	struct prio_queue queue = { NULL };
> +
> +	if (!commit || (commit->object.flags & COMMON))
> +		return;
> +
> +	prio_queue_put(&queue, commit);
> +	if (!ancestors_only) {
> +		commit->object.flags |= COMMON;
>  
> -		if (!ancestors_only)
> -			o->flags |= COMMON;
> +		if ((commit->object.flags & SEEN) && !(commit->object.flags & POPPED))
> +			ns->non_common_revs--;
> +	}
> +	while ((commit = prio_queue_get(&queue))) {
> +		struct object *o = (struct object *)commit;
>  
>  		if (!(o->flags & SEEN))
>  			rev_list_push(ns, commit, SEEN);
>  		else {
>  			struct commit_list *parents;
>  
> -			if (!ancestors_only && !(o->flags & POPPED))
> -				ns->non_common_revs--;
>  			if (!o->parsed && !dont_parse)
>  				if (repo_parse_commit(the_repository, commit))
> -					return;
> +					continue;
>  
>  			for (parents = commit->parents;
>  					parents;
> -					parents = parents->next)
> -				mark_common(ns, parents->item, 0,
> -					    dont_parse);
> +					parents = parents->next) {
> +				struct commit *p = parents->item;
> +
> +				if (p->object.flags & COMMON)
> +					continue;
> +
> +				p->object.flags |= COMMON;
> +
> +				if ((p->object.flags & SEEN) && !(p->object.flags & POPPED))
> +					ns->non_common_revs--;
> +
> +				prio_queue_put(&queue, parents->item);
> +			}
>  		}
>  	}
> +
> +	clear_prio_queue(&queue);

Thanks for this version. It looks like an identical set of actions in the commit walk, but the change from DFS to priority queue is a welcome change.

-Stolee
Previous: Han XinNext: Han Xin
Message 10 of 20 in “negotiator/default.c: avoid stack overflow”
  1. negotiator/default.c: avoid stack overflowHan Xin, Apr 24, 2023
  2. Derrick StoleeApr 24, 2023
  3. Han XinApr 25, 2023
  4. Derrick StoleeApr 25, 2023
  5. 0/2 negotiator/default: avoid stack overflowHan Xin, Apr 26, 2023
  6. 2/2 negotiator/skipping: fix some problems in mark_common()Han Xin, Apr 26, 2023
  7. Derrick StoleeApr 26, 2023
  8. Han XinApr 26, 2023
  9. 1/2 negotiator/default: avoid stack overflowHan Xin, Apr 26, 2023
  10. Derrick StoleeApr 26, 2023
  11. Han XinApr 26, 2023
  12. 0/2 negotiator/default: avoid stack overflowHan Xin, Apr 26, 2023
  13. 1/2 negotiator/default: avoid stack overflowHan Xin, Apr 26, 2023
  14. Junio C HamanoApr 26, 2023
  15. Derrick StoleeApr 26, 2023
  16. Junio C HamanoApr 26, 2023
  17. 2/2 negotiator/skipping: fix some problems in mark_common()Han Xin, Apr 26, 2023
  18. Junio C HamanoMay 1, 2023
  19. Derrick StoleeMay 2, 2023
  20. Junio C HamanoMay 2, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.