To: XEmacs Patches <xemacs-patches@xemacs.org>
Organization: The XEmacs Project
From: "Stephen J. Turnbull" <stephen@xemacs.org>
Date: Thu, 27 Jan 2005 17:36:25 +0900
Message-ID: <87fz0n1d6e.fsf@tleepslib.sk.tsukuba.ac.jp>
User-Agent: Gnus/5.1006 (Gnus v5.10.6) XEmacs/21.5 (chestnut, linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Andrey Slusar <anray@inet.ua>, Vin Shelton <acs@xemacs.org>,
	Arnaud Giersch <arnaud.giersch@free.fr>
Subject: [R21.4] Arnaud Giersch's patch for regex.c / fix Gnus infloop
Precedence: list
Sender: xemacs-patches-bounces@xemacs.org
Errors-To: xemacs-patches-bounces@xemacs.org
Lines: 243
Xref: zion.rcn.com XEmacs-review:12996

RECOMMEND 21.4

Not needed for 21.5 (it's already in 21.5; this has been a "forehead,
meet palm" experience :-P ).

This is Arnaud Giersch's patch to fix the Gnus infloop caused by my
errors in backporting the 21.5 shy groups code.  Andrey, you were
looking at almost the right place!  I'd appreciate it if you could
check, too, although Arnaud is surely looking at the same infloop that
you reported.

Besides being in 21.5 and theoretically correct, Arnaud says it fixes
the infloop for him.  And it's a fencepost-like error, as I suggested.
I'm sure this is it, although I can't test easily.

This _might_ fix _some_ of the "wrong side of point" errors and so on:
they could be due to accessing stale match register data, which is
what this patch prevents.  But I think some of those were previous to
application of the backport of 21.5's shy groups, so don't get your
hopes up too high.

Thanks, Arnaud!  I've added the ChangeLog, interchanged the
initialization and the comment at the top of the block, and
regularized the whitespace with `indent-region' (it's all tabs, now).
Sorry, Arnaud, this will cause a conflict when you CVS update.

For reference purposes, here is the semantic content of the patch
(cvs diff -w src/regex.c).

| Index: src/regex.c
| ===================================================================
| RCS file: /pack/xemacscvs/XEmacs/xemacs/src/regex.c,v
| retrieving revision 1.25.2.11
| diff -u -r1.25.2.11 regex.c
| --- src/regex.c	16 Jan 2005 02:38:58 -0000	1.25.2.11
| +++ src/regex.c	27 Jan 2005 08:15:23 -0000
| @@ -4714,10 +4714,11 @@
|  	succeed_label:
|            DEBUG_PRINT1 ("Accepting match.\n");
|  
| +	  {
|            /* If caller wants register contents data back, fill REGS.  */
| +	    int num_nonshy_regs = bufp->re_nsub + 1;
|  	  if (regs && !bufp->no_sub)
|  	    {
| -	      int num_nonshy_regs = bufp->re_nsub + 1;
|  	      /* Have the register data arrays been allocated?  */
|  	      if (bufp->regs_allocated == REGS_UNALLOCATED)
|  		{ /* No.  So allocate them with malloc.  We need one
| @@ -4801,8 +4802,9 @@
|  	     have to change the API for this function to reflect that, and
|  	     audit all callers. */
|  	  if (regs && regs->num_regs > 0)
| -	    for (mcnt = num_regs; mcnt < regs->num_regs; mcnt++)
| +	    for (mcnt = num_nonshy_regs; mcnt < regs->num_regs; mcnt++)
|  	      regs->start[mcnt] = regs->end[mcnt] = -1;
| +	  }
|  
|  	  DEBUG_PRINT4 ("%u failure points pushed, %u popped (%u remain).\n",
|  			nfailure_points_pushed, nfailure_points_popped,

Here are the ChangeLog and Arnaud's patch:

Index: src/ChangeLog
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/ChangeLog,v
retrieving revision 1.290.2.89
diff -u -U0 -r1.290.2.89 ChangeLog
--- src/ChangeLog	25 Jan 2005 01:15:27 -0000	1.290.2.89
+++ src/ChangeLog	27 Jan 2005 08:13:58 -0000
@@ -0,0 +1,5 @@
+2005-01-26  Arnaud Giersch  <arnaud.giersch@free.fr>
+
+	* regex.c (re_match_2_internal):
+	Correctly initialize loop which clears uninitialized registers.
+

Index: src/regex.c
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/regex.c,v
retrieving revision 1.25.2.11
diff -u -r1.25.2.11 regex.c
--- src/regex.c	16 Jan 2005 02:38:58 -0000	1.25.2.11
+++ src/regex.c	27 Jan 2005 08:24:39 -0000
@@ -4714,58 +4714,59 @@
 	succeed_label:
           DEBUG_PRINT1 ("Accepting match.\n");
 
-          /* If caller wants register contents data back, fill REGS.  */
-	  if (regs && !bufp->no_sub)
-	    {
-	      int num_nonshy_regs = bufp->re_nsub + 1;
-	      /* Have the register data arrays been allocated?  */
-	      if (bufp->regs_allocated == REGS_UNALLOCATED)
-		{ /* No.  So allocate them with malloc.  We need one
-		     extra element beyond `num_regs' for the `-1' marker
-		     GNU code uses.  */
-		  regs->num_regs = MAX (RE_NREGS, num_nonshy_regs + 1);
-		  regs->start = TALLOC (regs->num_regs, regoff_t);
-		  regs->end = TALLOC (regs->num_regs, regoff_t);
-		  if (regs->start == NULL || regs->end == NULL)
-		    {
-		      FREE_VARIABLES ();
-		      return -2;
-		    }
-		  bufp->regs_allocated = REGS_REALLOCATE;
-		}
-	      else if (bufp->regs_allocated == REGS_REALLOCATE)
-		{ /* Yes.  If we need more elements than were already
-		     allocated, reallocate them.  If we need fewer, just
-		     leave it alone.  */
-		  if (regs->num_regs < num_nonshy_regs + 1)
-		    {
-		      regs->num_regs = num_nonshy_regs + 1;
-		      RETALLOC (regs->start, regs->num_regs, regoff_t);
-		      RETALLOC (regs->end, regs->num_regs, regoff_t);
-		      if (regs->start == NULL || regs->end == NULL)
-			{
-			  FREE_VARIABLES ();
-			  return -2;
-			}
-		    }
-		}
-	      else
-		{
-		  /* The braces fend off a "empty body in an else-statement"
-		     warning under GCC when assert expands to nothing.  */
-		  assert (bufp->regs_allocated == REGS_FIXED);
-		}
-
-	      /* Convert the pointer data in `regstart' and `regend' to
-		 indices.  Register zero has to be set differently,
-		 since we haven't kept track of any info for it.  */
-	      if (regs->num_regs > 0)
-		{
-		  regs->start[0] = pos;
-		  regs->end[0] = (MATCHING_IN_FIRST_STRING
-				  ? ((regoff_t) (d - string1))
-				  : ((regoff_t) (d - string2 + size1)));
-		}
+	  {
+	    /* If caller wants register contents data back, fill REGS.  */
+	    int num_nonshy_regs = bufp->re_nsub + 1;
+	    if (regs && !bufp->no_sub)
+	      {
+		/* Have the register data arrays been allocated?  */
+		if (bufp->regs_allocated == REGS_UNALLOCATED)
+		  { /* No.  So allocate them with malloc.  We need one
+		       extra element beyond `num_regs' for the `-1' marker
+		       GNU code uses.  */
+		    regs->num_regs = MAX (RE_NREGS, num_nonshy_regs + 1);
+		    regs->start = TALLOC (regs->num_regs, regoff_t);
+		    regs->end = TALLOC (regs->num_regs, regoff_t);
+		    if (regs->start == NULL || regs->end == NULL)
+		      {
+			FREE_VARIABLES ();
+			return -2;
+		      }
+		    bufp->regs_allocated = REGS_REALLOCATE;
+		  }
+		else if (bufp->regs_allocated == REGS_REALLOCATE)
+		  { /* Yes.  If we need more elements than were already
+		       allocated, reallocate them.  If we need fewer, just
+		       leave it alone.  */
+		    if (regs->num_regs < num_nonshy_regs + 1)
+		      {
+			regs->num_regs = num_nonshy_regs + 1;
+			RETALLOC (regs->start, regs->num_regs, regoff_t);
+			RETALLOC (regs->end, regs->num_regs, regoff_t);
+			if (regs->start == NULL || regs->end == NULL)
+			  {
+			    FREE_VARIABLES ();
+			    return -2;
+			  }
+		      }
+		  }
+		else
+		  {
+		    /* The braces fend off a "empty body in an else-statement"
+		       warning under GCC when assert expands to nothing.  */
+		    assert (bufp->regs_allocated == REGS_FIXED);
+		  }
+
+		/* Convert the pointer data in `regstart' and `regend' to
+		   indices.  Register zero has to be set differently,
+		   since we haven't kept track of any info for it.  */
+		if (regs->num_regs > 0)
+		  {
+		    regs->start[0] = pos;
+		    regs->end[0] = (MATCHING_IN_FIRST_STRING
+				    ? ((regoff_t) (d - string1))
+				    : ((regoff_t) (d - string2 + size1)));
+		  }
 
 		/* Map over the NUM_NONSHY_REGS non-shy internal registers.
 		   Copy each into the corresponding external register.
@@ -4788,21 +4789,22 @@
 		  }
 	      } /* regs && !bufp->no_sub */
 
-	  /* If we have regs and the regs structure has more elements than
-	     were in the pattern, set the extra elements to -1.  If we
-	     (re)allocated the registers, this is the case, because we
-	     always allocate enough to have at least one -1 at the end.
-
-	     We do this even when no_sub is set because some applications
-	     (XEmacs) reuse register structures which may contain stale
-	     information, and permit attempts to access those registers.
-
-	     It would be possible to require the caller to do this, but we'd
-	     have to change the API for this function to reflect that, and
-	     audit all callers. */
-	  if (regs && regs->num_regs > 0)
-	    for (mcnt = num_regs; mcnt < regs->num_regs; mcnt++)
-	      regs->start[mcnt] = regs->end[mcnt] = -1;
+	    /* If we have regs and the regs structure has more elements than
+	       were in the pattern, set the extra elements to -1.  If we
+	       (re)allocated the registers, this is the case, because we
+	       always allocate enough to have at least one -1 at the end.
+
+	       We do this even when no_sub is set because some applications
+	       (XEmacs) reuse register structures which may contain stale
+	       information, and permit attempts to access those registers.
+
+	       It would be possible to require the caller to do this, but we'd
+	       have to change the API for this function to reflect that, and
+	       audit all callers. */
+	    if (regs && regs->num_regs > 0)
+	      for (mcnt = num_nonshy_regs; mcnt < regs->num_regs; mcnt++)
+		regs->start[mcnt] = regs->end[mcnt] = -1;
+	  }
 
 	  DEBUG_PRINT4 ("%u failure points pushed, %u popped (%u remain).\n",
 			nfailure_points_pushed, nfailure_points_popped,


-- 
Institute of Policy and Planning Sciences     http://turnbull.sk.tsukuba.ac.jp
University of Tsukuba                    Tennodai 1-1-1 Tsukuba 305-8573 JAPAN
               Ask not how you can "do" free software business;
              ask what your business can "do for" free software.