bug#24358: 25.1.50; re-search-forward errors with "Variable binding depth exceeds max-specpdl-size"

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: npostavs@users.sourceforge.net
>> Cc: 24358@debbugs.gnu.org,  peder@klingenberg.no
>> Date: Sat, 08 Oct 2016 09:45:20 -0400
>> 
>> >> From: npostavs@users.sourceforge.net
>> >> Date: Fri, 07 Oct 2016 20:29:36 -0400
>> >> Cc: 24358@debbugs.gnu.org
>> >> 
>> >> npostavs@users.sourceforge.net writes:
>> >> >
>> >> >> (I'm also on GNU/Linux, Arch) I get the same max-specpdl-size error 
>> >> >> with
>> >> >> 25.1.50, with 24.5 (and below) I get (error "Stack overflow in regexp
>> >> >> matcher")
>> >> 
>> >> icalendar--read-element has been fixed, but this still reproduces when
>> >> doing (re-search-forward ".*\\(\n.*\\)*" nil t) on the text file given
>> >> in the OP.
>> >
>> > Isn't that "user error"?
>> 
>> Yes, but it should give "Stack overflow in regexp matcher", not overflow
>> the lisp stack (or assertion failure).
>
> But that's what you said (see above): "Stack overflow in regexp
> matcher".  That's what I meant when I said "user error".

Ah, I may have been a bit too terse there.  What I meant was, in Emacs
24.5 I correctly get "Stack overflow in regexp matcher", whereas in
emacs-master I get "Variable binding depth exceeds max-specpdl-size".
In emacs-25 I get the assertion failure.

>
>> I show some more excerpts in the attached bug-24358-debug.log, but my
>> main finding is that string1 of re_match_2_internal is originally:
>> 
>>     string1=0x1835980 "DESCRIPTION;LANGUAGE=
>> 
>> but then it becomes corrupted during a malloc:
>> 
>> Old value = 68 'D'
>> New value = 0 '\000'
>
> If that string is data of a Lisp string, then a call to malloc could
> relocate the data.  Code that holds C pointers into buffer or string
> text should either use SREF, or recompute the C pointer after each
> function call which could GC.

In that case, I believe the problem is that search_buffer calls
re_search_2 with a pointer to the buffer text, and then
re_match_2_internal (called by re_search_2), can allocate when doing
PUSH_FAILURE_POINT because it eventually does SAFE_ALLOCA to grow the
regex stack.

AFAICT, this bug is still present 24.5, but because re_max_failures is
set to a round number (see
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24358#27), there are fewer
calls to malloc and thus less chance of relocating the particular string
in question.

So possible solutions I can would be to pass down the lisp reference to
re_match_2_internal, or else set re_max_failures according to MAX_ALLOCA
(but this would make it much smaller).

search_buffer()
      /* Get pointers and sizes of the two strings
         that make up the visible portion of the buffer. */

      p1 = BEGV_ADDR;
      s1 = GPT_BYTE - BEGV_BYTE;
      p2 = GAP_END_ADDR;
      s2 = ZV_BYTE - GPT_BYTE;

[...]

          val = re_search_2 (bufp, (char *) p1, s1, (char *) p2, s2,
                             pos_byte - BEGV_BYTE, lim_byte - pos_byte,
                             (NILP (Vinhibit_changing_match_data)
                              ? &search_regs : &search_regs_1),
                             /* Don't allow match past current point */
                             pos_byte - BEGV_BYTE);

re_match_2_internal()
        case on_failure_jump:
          EXTRACT_NUMBER_AND_INCR (mcnt, p);
          DEBUG_PRINT ("EXECUTING on_failure_jump %d (to %p):\n",
                       mcnt, p + mcnt);

          PUSH_FAILURE_POINT (p -3, d);

#define PUSH_FAILURE_POINT(pattern, string_place)
do { ...
  ENSURE_FAIL_STACK (NUM_NONREG_ITEMS);...

#define ENSURE_FAIL_STACK(space)                                        \
while (REMAINING_AVAIL_SLOTS <= space) {                                \
  if (!GROW_FAIL_STACK (fail_stack))                                    \
    return -2;...

#define GROW_FAIL_STACK(fail_stack)                                     \
      ...
      = REGEX_REALLOCATE_STACK ((fail_stack).stack,                     \
          (fail_stack).size * sizeof (fail_stack_elt_t),                \
          min (re_max_failures * TYPICAL_FAILURE_SIZE,                  \
               ((fail_stack).size * sizeof (fail_stack_elt_t)           \
                * FAIL_STACK_GROWTH_FACTOR))),                          \

# define REGEX_ALLOCATE_STACK(size) REGEX_ALLOCATE (size)
# define REGEX_REALLOCATE_STACK(source, o, n) REGEX_REALLOCATE (source, o, n)

#  define REGEX_ALLOCATE SAFE_ALLOCA

/* SAFE_ALLOCA normally allocates memory on the stack, but if size is
   larger than MAX_ALLOCA, use xmalloc to avoid overflowing the stack.  */

enum MAX_ALLOCA { MAX_ALLOCA = 16 * 1024 };

#define SAFE_ALLOCA(size) ((size) < MAX_ALLOCA  \
                           ? alloca (size)      \
                           : (sa_must_free = true, record_xmalloc (size)))
                                       ^^^^^^^^^^^^^^^^^^^^^