Subject: bug#59029: 29.0.50; noverlay: pdumper.c: dump_interval_node recursion has no base case

From: Gerd Möllmann <[email-masked]>
Date: Sat, 05 Nov 2022 06:41:41 +0100

Matt Armstrong <[email-masked]> writes:

> X-Debbugs-cc: Stefan Monnier <[email-masked]>
>
> This has been in my head for weeks but I haven't had time to dig into
> it.  Best get it in a bug.
>
> See the code for dump_interval_node() in pdumper.c below.
>
> Imagine 'node' has a left child.  It will recurse to that child on line
> 35.  That child will recurse back to its parent on line 30.  That parent
> will recurse back to its left child on line 35.  This will repeat until
> the stack blows.  All you need is two nodes in the tree.
>
> This is not an immediate issue today because apparently Emacs does not
> dump any buffers with overlays present, or at least, never more than one
> overlay.  I suspect the right fix is to delete lines 26-30, or something
> like that, but I can't claim I understand this code.
>
>      1	static dump_off
>      2	dump_interval_node (struct dump_context *ctx, struct itree_node *node,
>      3	                    dump_off parent_offset)
>      4	{
>      5	#if CHECK_STRUCTS && !defined (HASH_itree_node_50DE304F13)
>      6	# error "itree_node changed. See CHECK_STRUCTS comment in config.h."
>      7	#endif
>      8	  struct itree_node out;
>      9	  dump_object_start (ctx, &out, sizeof (out));
>     10	  if (node->parent)
>     11	    dump_field_fixup_later (ctx, &out, node, &node->parent);
>     12	  if (node->left)
>     13	    dump_field_fixup_later (ctx, &out, node, &node->parent);
>     14	  if (node->right)
>     15	    dump_field_fixup_later (ctx, &out, node, &node->parent);
>     16	  DUMP_FIELD_COPY (&out, node, begin);
>     17	  DUMP_FIELD_COPY (&out, node, end);
>     18	  DUMP_FIELD_COPY (&out, node, limit);
>     19	  DUMP_FIELD_COPY (&out, node, offset);
>     20	  DUMP_FIELD_COPY (&out, node, otick);
>     21	  dump_field_lv (ctx, &out, node, &node->data, WEIGHT_STRONG);
>     22	  DUMP_FIELD_COPY (&out, node, red);
>     23	  DUMP_FIELD_COPY (&out, node, rear_advance);
>     24	  DUMP_FIELD_COPY (&out, node, front_advance);
>     25	  dump_off offset = dump_object_finish (ctx, &out, sizeof (out));
>     26	  if (node->parent)
>     27	      dump_remember_fixup_ptr_raw
>     28		(ctx,
>     29		 offset + dump_offsetof (struct itree_node, parent),
>     30		 dump_interval_node (ctx, node->parent, offset));
>     31	  if (node->left)
>     32	      dump_remember_fixup_ptr_raw
>     33		(ctx,
>     34		 offset + dump_offsetof (struct itree_node, left),
>     35		 dump_interval_node (ctx, node->left, offset));
>     36	  if (node->right)
>     37	      dump_remember_fixup_ptr_raw
>     38		(ctx,
>     39		 offset + dump_offsetof (struct itree_node, right),
>     40		 dump_interval_node (ctx, node->right, offset));
>     41	  return offset;
>     42	}

Yes, I think you are right.

Could we also rename dump_interval_node to dump_itree_node?  There is
another function dump_interval_tree for text properties, which is a bit
confusing.