mg: segmentation fault when using query-replace-regexp

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

mg: segmentation fault when using query-replace-regexp

Mark Willson-2
Folks,

The segmentation fault occurs when the string to replace is just the
"^" anchor (beginning-of-line) and the point is on an empty line.
Issue occurs on a -current system dated July 11th. (dmesg below).

The following patch prevents the segmentation fault:

--- /usr/src/usr.bin/mg/line.c  Mon Jul 13 13:31:14 2020
+++ line.c      Mon Jul 13 13:01:49 2020
@@ -556,6 +556,8 @@
                goto done;

        lp = curwp->w_dotp;
+       if ((lp)->l_text == NULL)
+               goto done;
        doto = curwp->w_doto;
        n = plen;

However, this reveals another defect.  The query-replace-regexp now
never advances beyond the current line and keeps inserting the
replacement text into it. If you've chosen ! (replace rest) mg
eventually runs out of memory.

This is fixed by the second modification to re_search.c shown below.  The
first
is required to enable mg to perform the replacement on the current line
when it is empty.

--- /usr/src/usr.bin/mg/re_search.c     Mon Jul 13 09:15:11 2020
+++ re_search.c Mon Jul 13 13:34:09 2020
@@ -318,9 +318,9 @@
        if (tbo == clp->l_used)
                /*
                 * Don't start matching past end of line -- must move to
-                * beginning of next line, unless at end of file.
+                * beginning of next line, unless at end of file or line
empty.
                 */
-               if (clp != curbp->b_headp) {
+               if (clp != curbp->b_headp && llength(clp) != 0) {
                        clp = lforw(clp);
                        tdotline++;
                        tbo = 0;
@@ -333,7 +333,7 @@
                regex_match[0].rm_so = tbo;
                regex_match[0].rm_eo = llength(clp);
                error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
-                   RE_NMATCH, regex_match, REG_STARTEND);
+                   RE_NMATCH, regex_match,
(tbo==0?0:REG_NOTBOL)|REG_STARTEND);
                if (error != 0) {
                        clp = lforw(clp);
                        tdotline++;


OpenBSD 6.7-current (GENERIC) #328: Sat Jul 11 20:12:48 MDT 2020
    [hidden email]:/usr/src/sys/arch/amd64/compile/GENERIC
real mem = 1056899072 (1007MB)
avail mem = 1009971200 (963MB)
random: good seed from bootblocks
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.3 @ 0xf93d0 (338 entries)
bios0: vendor American Megatrends Inc. version "090008" date 12/07/2018
bios0: Microsoft Corporation Virtual Machine
acpi0 at bios0: ACPI 2.0
acpi0: sleep states S0 S5
acpi0: tables DSDT FACP WAET SLIC OEM0 SRAT APIC OEMB
acpi0: wakeup devices
acpitimer0 at acpi0: 3579545 Hz, 32 bits
acpihve0 at acpi0
acpimadt0 at acpi0 addr 0xfee00000: PC-AT compat
ioapic0 at mainbus0: apid 0 pa 0xfec00000, version 11, 24 pins, remapped
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: AMD Ryzen 7 1700 Eight-Core Processor, 2619.63 MHz, 17-01-01
cpu0:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLU
SH,MMX,FXSR,SSE,SSE2,SSE3,PCLMUL,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,
AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,MMXX,FFXSR,PAGE1GB,RDTSCP,LONG,LAHF,CMPLEG,
AMCR8,ABM,SSE4A,MASSE,3DNOWP,OSVW,FSGSBASE,BMI1,AVX2,SMEP,BMI2,RDSEED,ADX,SM
AP,CLFLUSHOPT,SHA,IBPB,VIRTSSBD,XSAVEOPT,XSAVEC,XGETBV1,XSAVES
cpu0: 64KB 64b/line 4-way I-cache, 32KB 64b/line 8-way D-cache, 512KB
64b/line 8-way L2 cache, 16MB 64b/line 16-way L3 cache
cpu0: ITLB 64 4KB entries fully associative, 64 4MB entries fully
associative
cpu0: DTLB 64 4KB entries fully associative, 64 4MB entries fully
associative
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 169MHz
acpiprt0 at acpi0: bus 0 (PCI0)
acpicpu0 at acpi0: C1(@1 halt!)
acpipci0 at acpi0 PCI0
extent `acpipci0 pcibus' (0x0 - 0xff), flags=0
extent `pciio' (0x0 - 0xffffffff), flags=0
     0x10000 - 0xffffffff
extent `pcimem' (0x0 - 0xffffffffffffffff), flags=0
     0x0 - 0x3fffffff
     0x40000000000 - 0xffffffffffffffff
acpicmos0 at acpi0
"VMBus" at acpi0 not configured
"Hyper_V_Gen_Counter_V1" at acpi0 not configured
pvbus0 at mainbus0: Hyper-V 10.0
hyperv0 at pvbus0: protocol 4.0, features 0x2e7f
hyperv0: heartbeat, kvp, shutdown, timesync
hvs0 at hyperv0 channel 2: ide, protocol 6.2
scsibus1 at hvs0: 2 targets
sd0 at scsibus1 targ 0 lun 0: <Msft, Virtual Disk, 1.0>
naa.60022480e357104f5b7149127201f778
sd0: 25600MB, 512 bytes/sector, 52428800 sectors, thin
hvn0 at hyperv0 channel 14: NVS 5.0 NDIS 6.30, address 00:15:5d:00:10:06
hvs1 at hyperv0 channel 15: scsi, protocol 6.2
scsibus2 at hvs1: 2 targets
pci0 at mainbus0 bus 0
pchb0 at pci0 dev 0 function 0 "Intel 82443BX" rev 0x03
pcib0 at pci0 dev 7 function 0 "Intel 82371AB PIIX4 ISA" rev 0x01
pciide0 at pci0 dev 7 function 1 "Intel 82371AB IDE" rev 0x01: DMA, channel
0 wired to compatibility, channel 1 wired to compatibility
pciide0: channel 0 disabled (no drives)
atapiscsi0 at pciide0 channel 1 drive 0
scsibus3 at atapiscsi0: 2 targets
cd0 at scsibus3 targ 0 lun 0: <Msft, Virtual CD/ROM, 1.0> removable
cd0(pciide0:1:0): using PIO mode 4, DMA mode 2
piixpm0 at pci0 dev 7 function 3 "Intel 82371AB Power" rev 0x02: SMBus
disabled
vga1 at pci0 dev 8 function 0 "Microsoft VGA" rev 0x00
wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation)
wsdisplay0: screen 1-5 added (80x25, vt100 emulation)
isa0 at pcib0
isadma0 at isa0
fdc0 at isa0 port 0x3f0/6 irq 6 drq 2
com0 at isa0 port 0x3f8/8 irq 4: ns16550a, 16 byte fifo
com1 at isa0 port 0x2f8/8 irq 3: ns16550a, 16 byte fifo
pckbc0 at isa0 port 0x60/5 irq 1 irq 12
pckbd0 at pckbc0 (kbd slot)
wskbd0 at pckbd0: console keyboard, using wsdisplay0
pms0 at pckbc0 (aux slot)
wsmouse0 at pms0 mux 0
pcppi0 at isa0 port 0x61
spkr0 at pcppi0
vscsi0 at root
scsibus4 at vscsi0: 256 targets
softraid0 at root
scsibus5 at softraid0: 256 targets
root on sd0a (7f9b785a6981db5a.a) swap on sd0b dump on sd0b
fd0 at fdc0 drive 0: 1.44MB 80 cyl, 2 head, 18 sec
fd1 at fdc0 drive 1: density unknown
acpi0: PM1 stuck (en 0x101 st 0x1), clearing

Thanks,
-mark
--
Mark Willson
[hidden email]
https://hydrus.org.uk


Reply | Threaded
Open this post in threaded view
|

Re: mg: segmentation fault when using query-replace-regexp

Theo Buehler-3
On Mon, Jul 13, 2020 at 03:47:13PM +0100, Mark Willson wrote:
> Folks,
>
> The segmentation fault occurs when the string to replace is just the
> "^" anchor (beginning-of-line) and the point is on an empty line.
> Issue occurs on a -current system dated July 11th. (dmesg below).
>
> The following patch prevents the segmentation fault:

Nice. Thanks for the patches and the analysis. I'd suggest the following
variant (only stylistic changes).  I'm willing to commit this, but I
will need at least some positive feedback from mg users.

Index: line.c
===================================================================
RCS file: /var/cvs/src/usr.bin/mg/line.c,v
retrieving revision 1.61
diff -u -p -r1.61 line.c
--- line.c 29 Aug 2018 07:50:16 -0000 1.61
+++ line.c 13 Jul 2020 16:00:52 -0000
@@ -556,6 +556,8 @@ lreplace(RSIZE plen, char *st)
  goto done;
 
  lp = curwp->w_dotp;
+ if (ltext(lp) == NULL)
+ goto done;
  doto = curwp->w_doto;
  n = plen;
 
Index: re_search.c
===================================================================
RCS file: /var/cvs/src/usr.bin/mg/re_search.c,v
retrieving revision 1.34
diff -u -p -r1.34 re_search.c
--- re_search.c 9 Jul 2020 10:42:24 -0000 1.34
+++ re_search.c 13 Jul 2020 16:17:07 -0000
@@ -308,7 +308,7 @@ re_doreplace(RSIZE plen, char *st)
 static int
 re_forwsrch(void)
 {
- int tbo, tdotline, error;
+ int re_flags, tbo, tdotline, error;
  struct line *clp;
 
  clp = curwp->w_dotp;
@@ -318,9 +318,10 @@ re_forwsrch(void)
  if (tbo == clp->l_used)
  /*
  * Don't start matching past end of line -- must move to
- * beginning of next line, unless at end of file.
+ * beginning of next line, unless line is empty or at
+ * end of file.
  */
- if (clp != curbp->b_headp) {
+ if (clp != curbp->b_headp && llength(clp) != 0) {
  clp = lforw(clp);
  tdotline++;
  tbo = 0;
@@ -330,10 +331,13 @@ re_forwsrch(void)
  * always makes the last line empty so this is good.
  */
  while (clp != (curbp->b_headp)) {
+ re_flags = REG_STARTEND;
+ if (tbo != 0)
+ re_flags |= REG_NOTBOL;
  regex_match[0].rm_so = tbo;
  regex_match[0].rm_eo = llength(clp);
  error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
-    RE_NMATCH, regex_match, REG_STARTEND);
+    RE_NMATCH, regex_match, re_flags);
  if (error != 0) {
  clp = lforw(clp);
  tdotline++;

Reply | Threaded
Open this post in threaded view
|

Re: mg: segmentation fault when using query-replace-regexp

Mark Willson-2
> -----Original Message-----
> From: Theo Buehler <[hidden email]>
> Sent: 20 July 2020 21:12
> To: Mark Willson <[hidden email]>
> Cc: [hidden email]; [hidden email]
> Subject: Re: mg: segmentation fault when using query-replace-regexp
>
> On Mon, Jul 13, 2020 at 03:47:13PM +0100, Mark Willson wrote:
> > Folks,
> >
> > The segmentation fault occurs when the string to replace is just the
> > "^" anchor (beginning-of-line) and the point is on an empty line.
> > Issue occurs on a -current system dated July 11th. (dmesg below).
> >
> > The following patch prevents the segmentation fault:
>
> Nice. Thanks for the patches and the analysis. I'd suggest the following
> variant (only stylistic changes).  I'm willing to commit this, but I
> will need at least some positive feedback from mg users.
>
> Index: line.c
> ===================================================================
> RCS file: /var/cvs/src/usr.bin/mg/line.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 line.c
> --- line.c 29 Aug 2018 07:50:16 -0000 1.61
> +++ line.c 13 Jul 2020 16:00:52 -0000
> @@ -556,6 +556,8 @@ lreplace(RSIZE plen, char *st)
>   goto done;
>
>   lp = curwp->w_dotp;
> + if (ltext(lp) == NULL)
> + goto done;
>   doto = curwp->w_doto;
>   n = plen;
>
> Index: re_search.c
> ===================================================================
> RCS file: /var/cvs/src/usr.bin/mg/re_search.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 re_search.c
> --- re_search.c 9 Jul 2020 10:42:24 -0000 1.34
> +++ re_search.c 13 Jul 2020 16:17:07 -0000
> @@ -308,7 +308,7 @@ re_doreplace(RSIZE plen, char *st)
>  static int
>  re_forwsrch(void)
>  {
> - int tbo, tdotline, error;
> + int re_flags, tbo, tdotline, error;
>   struct line *clp;
>
>   clp = curwp->w_dotp;
> @@ -318,9 +318,10 @@ re_forwsrch(void)
>   if (tbo == clp->l_used)
>   /*
>   * Don't start matching past end of line -- must move to
> - * beginning of next line, unless at end of file.
> + * beginning of next line, unless line is empty or at
> + * end of file.
>   */
> - if (clp != curbp->b_headp) {
> + if (clp != curbp->b_headp && llength(clp) != 0) {
>   clp = lforw(clp);
>   tdotline++;
>   tbo = 0;
> @@ -330,10 +331,13 @@ re_forwsrch(void)
>   * always makes the last line empty so this is good.
>   */
>   while (clp != (curbp->b_headp)) {
> + re_flags = REG_STARTEND;
> + if (tbo != 0)
> + re_flags |= REG_NOTBOL;
>   regex_match[0].rm_so = tbo;
>   regex_match[0].rm_eo = llength(clp);
>   error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
> -    RE_NMATCH, regex_match, REG_STARTEND);
> +    RE_NMATCH, regex_match, re_flags);
>   if (error != 0) {
>   clp = lforw(clp);
>   tdotline++;

Theo,

Thanks for the response.  I do hope the patch can be committed; it's always
unnerving when an editor dies underneath you.

-mark
--
Mark Willson
[hidden email]
https://hydrus.org.uk

Reply | Threaded
Open this post in threaded view
|

Re: mg: segmentation fault when using query-replace-regexp

Hiltjo Posthuma
In reply to this post by Theo Buehler-3
On Mon, Jul 20, 2020 at 10:11:46PM +0200, Theo Buehler wrote:
> On Mon, Jul 13, 2020 at 03:47:13PM +0100, Mark Willson wrote:
> > Folks,
> >
> > The segmentation fault occurs when the string to replace is just the
> > "^" anchor (beginning-of-line) and the point is on an empty line.
> > Issue occurs on a -current system dated July 11th. (dmesg below).
> >

I can reproduce the issue, but I don't get a segmentation fault but an infinite
loop here on -current and on 6.6 also (it is not a regression from the previous
patch).

> > The following patch prevents the segmentation fault:
>
> Nice. Thanks for the patches and the analysis. I'd suggest the following
> variant (only stylistic changes).  I'm willing to commit this, but I
> will need at least some positive feedback from mg users.
>
> Index: line.c
> ===================================================================
> RCS file: /var/cvs/src/usr.bin/mg/line.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 line.c
> --- line.c 29 Aug 2018 07:50:16 -0000 1.61
> +++ line.c 13 Jul 2020 16:00:52 -0000
> @@ -556,6 +556,8 @@ lreplace(RSIZE plen, char *st)
>   goto done;
>  
>   lp = curwp->w_dotp;
> + if (ltext(lp) == NULL)
> + goto done;
>   doto = curwp->w_doto;
>   n = plen;
>  
> Index: re_search.c
> ===================================================================
> RCS file: /var/cvs/src/usr.bin/mg/re_search.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 re_search.c
> --- re_search.c 9 Jul 2020 10:42:24 -0000 1.34
> +++ re_search.c 13 Jul 2020 16:17:07 -0000
> @@ -308,7 +308,7 @@ re_doreplace(RSIZE plen, char *st)
>  static int
>  re_forwsrch(void)
>  {
> - int tbo, tdotline, error;
> + int re_flags, tbo, tdotline, error;
>   struct line *clp;
>  
>   clp = curwp->w_dotp;
> @@ -318,9 +318,10 @@ re_forwsrch(void)
>   if (tbo == clp->l_used)
>   /*
>   * Don't start matching past end of line -- must move to
> - * beginning of next line, unless at end of file.
> + * beginning of next line, unless line is empty or at
> + * end of file.
>   */

I think the below line should stay the same, else forward searching on
empty lines does not work:

> - if (clp != curbp->b_headp) {
> + if (clp != curbp->b_headp && llength(clp) != 0) {


>   clp = lforw(clp);
>   tdotline++;
>   tbo = 0;
> @@ -330,10 +331,13 @@ re_forwsrch(void)
>   * always makes the last line empty so this is good.
>   */
>   while (clp != (curbp->b_headp)) {
> + re_flags = REG_STARTEND;
> + if (tbo != 0)
> + re_flags |= REG_NOTBOL;
>   regex_match[0].rm_so = tbo;
>   regex_match[0].rm_eo = llength(clp);
>   error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
> -    RE_NMATCH, regex_match, REG_STARTEND);
> +    RE_NMATCH, regex_match, re_flags);
>   if (error != 0) {
>   clp = lforw(clp);
>   tdotline++;
>

Tested on -current and with the above change looks good to me, thanks!

--
Kind regards,
Hiltjo

Reply | Threaded
Open this post in threaded view
|

Re: mg: segmentation fault when using query-replace-regexp

Kenneth Westerback
On Tue, Jul 21, 2020 at 11:55:39AM +0200, Hiltjo Posthuma wrote:

> On Mon, Jul 20, 2020 at 10:11:46PM +0200, Theo Buehler wrote:
> > On Mon, Jul 13, 2020 at 03:47:13PM +0100, Mark Willson wrote:
> > > Folks,
> > >
> > > The segmentation fault occurs when the string to replace is just the
> > > "^" anchor (beginning-of-line) and the point is on an empty line.
> > > Issue occurs on a -current system dated July 11th. (dmesg below).
> > >
>
> I can reproduce the issue, but I don't get a segmentation fault but an infinite
> loop here on -current and on 6.6 also (it is not a regression from the previous
> patch).
>

Hmm. I get neither a seg fault nor a loop on my amd64 Lenove E595. The
cursor moves to different spots in X/i3 xterm vs console and in either
case doesn't do any replacement.

.... Ken


> > > The following patch prevents the segmentation fault:
> >
> > Nice. Thanks for the patches and the analysis. I'd suggest the following
> > variant (only stylistic changes).  I'm willing to commit this, but I
> > will need at least some positive feedback from mg users.
> >
> > Index: line.c
> > ===================================================================
> > RCS file: /var/cvs/src/usr.bin/mg/line.c,v
> > retrieving revision 1.61
> > diff -u -p -r1.61 line.c
> > --- line.c 29 Aug 2018 07:50:16 -0000 1.61
> > +++ line.c 13 Jul 2020 16:00:52 -0000
> > @@ -556,6 +556,8 @@ lreplace(RSIZE plen, char *st)
> >   goto done;
> >  
> >   lp = curwp->w_dotp;
> > + if (ltext(lp) == NULL)
> > + goto done;
> >   doto = curwp->w_doto;
> >   n = plen;
> >  
> > Index: re_search.c
> > ===================================================================
> > RCS file: /var/cvs/src/usr.bin/mg/re_search.c,v
> > retrieving revision 1.34
> > diff -u -p -r1.34 re_search.c
> > --- re_search.c 9 Jul 2020 10:42:24 -0000 1.34
> > +++ re_search.c 13 Jul 2020 16:17:07 -0000
> > @@ -308,7 +308,7 @@ re_doreplace(RSIZE plen, char *st)
> >  static int
> >  re_forwsrch(void)
> >  {
> > - int tbo, tdotline, error;
> > + int re_flags, tbo, tdotline, error;
> >   struct line *clp;
> >  
> >   clp = curwp->w_dotp;
> > @@ -318,9 +318,10 @@ re_forwsrch(void)
> >   if (tbo == clp->l_used)
> >   /*
> >   * Don't start matching past end of line -- must move to
> > - * beginning of next line, unless at end of file.
> > + * beginning of next line, unless line is empty or at
> > + * end of file.
> >   */
>
> I think the below line should stay the same, else forward searching on
> empty lines does not work:
>
> > - if (clp != curbp->b_headp) {
> > + if (clp != curbp->b_headp && llength(clp) != 0) {
>
>
> >   clp = lforw(clp);
> >   tdotline++;
> >   tbo = 0;
> > @@ -330,10 +331,13 @@ re_forwsrch(void)
> >   * always makes the last line empty so this is good.
> >   */
> >   while (clp != (curbp->b_headp)) {
> > + re_flags = REG_STARTEND;
> > + if (tbo != 0)
> > + re_flags |= REG_NOTBOL;
> >   regex_match[0].rm_so = tbo;
> >   regex_match[0].rm_eo = llength(clp);
> >   error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
> > -    RE_NMATCH, regex_match, REG_STARTEND);
> > +    RE_NMATCH, regex_match, re_flags);
> >   if (error != 0) {
> >   clp = lforw(clp);
> >   tdotline++;
> >
>
> Tested on -current and with the above change looks good to me, thanks!
>
> --
> Kind regards,
> Hiltjo
>

Reply | Threaded
Open this post in threaded view
|

Re: mg: segmentation fault when using query-replace-regexp

Mark Willson-2
In reply to this post by Hiltjo Posthuma


> -----Original Message-----
> From: Hiltjo Posthuma <[hidden email]>
> Sent: 21 July 2020 10:56
> > Index: re_search.c
> > ===================================================================
> > RCS file: /var/cvs/src/usr.bin/mg/re_search.c,v
> > retrieving revision 1.34
> > diff -u -p -r1.34 re_search.c
> > --- re_search.c 9 Jul 2020 10:42:24 -0000 1.34
> > +++ re_search.c 13 Jul 2020 16:17:07 -0000
> > @@ -308,7 +308,7 @@ re_doreplace(RSIZE plen, char *st)
> >  static int
> >  re_forwsrch(void)
> >  {
> > - int tbo, tdotline, error;
> > + int re_flags, tbo, tdotline, error;
> >   struct line *clp;
> >
> >   clp = curwp->w_dotp;
> > @@ -318,9 +318,10 @@ re_forwsrch(void)
> >   if (tbo == clp->l_used)
> >   /*
> >   * Don't start matching past end of line -- must move to
> > - * beginning of next line, unless at end of file.
> > + * beginning of next line, unless line is empty or at
> > + * end of file.
> >   */
>
> I think the below line should stay the same, else forward searching on
> empty lines does not work:
>
> > - if (clp != curbp->b_headp) {
> > + if (clp != curbp->b_headp && llength(clp) != 0) {
>
>

Hiltjo,

Thanks very much for your review.  What you point out above is certainly
true, but
then it is consistent with the current behaviour when searching for the
beginning-of-line ('^') anchor and the point at the beginning of a
non-empty line. In that case, the point does not move.

-mark
--
Mark Willson
[hidden email]
https://hydrus.org.uk

Reply | Threaded
Open this post in threaded view
|

Re: mg: segmentation fault when using query-replace-regexp

Theo Buehler-3
In reply to this post by Kenneth Westerback
> Hmm. I get neither a seg fault nor a loop on my amd64 Lenove E595. The
> cursor moves to different spots in X/i3 xterm vs console and in either
> case doesn't do any replacement.

I can reproduce both behaviors reliably as follows both on console and
in xterm.

$ echo 'a\n\n\nb' > /tmp/test
$ mg /tmp/test

Don't move the cursor, just do the following (what is between quotation
marks is what you are supposed to type, the other bits are prompts):

"M-x query-replace-regexp<enter>"
RE Query replace: "^<enter>"
Query replace ^ with: "a<enter>"
Query replacing ^ with a: "<enter>"
<SP> replace, [.] rep-end, <DEL> don't, [!] repl rest <ESC> quit "!"

After hitting "!" here, it will enter a loop and eventually print

panic: Out of memory in undo code (record)

and exit.

If I do the exact same dance after moving the cursor to the second
(empty) line, it will segfault.

With the diff below (which is what I understood to be Hiltjo's
suggestion), I get more or less what I would expect. I think it should
not replace the empty line after the last newline...

So I think both diffs are not quite right :)

Index: line.c
===================================================================
RCS file: /var/cvs/src/usr.bin/mg/line.c,v
retrieving revision 1.61
diff -u -p -r1.61 line.c
--- line.c 29 Aug 2018 07:50:16 -0000 1.61
+++ line.c 21 Jul 2020 14:58:49 -0000
@@ -556,6 +556,8 @@ lreplace(RSIZE plen, char *st)
  goto done;
 
  lp = curwp->w_dotp;
+ if (ltext(lp) == NULL)
+ goto done;
  doto = curwp->w_doto;
  n = plen;
 
Index: re_search.c
===================================================================
RCS file: /var/cvs/src/usr.bin/mg/re_search.c,v
retrieving revision 1.34
diff -u -p -r1.34 re_search.c
--- re_search.c 9 Jul 2020 10:42:24 -0000 1.34
+++ re_search.c 21 Jul 2020 15:00:08 -0000
@@ -308,7 +308,7 @@ re_doreplace(RSIZE plen, char *st)
 static int
 re_forwsrch(void)
 {
- int tbo, tdotline, error;
+ int re_flags, tbo, tdotline, error;
  struct line *clp;
 
  clp = curwp->w_dotp;
@@ -330,10 +330,13 @@ re_forwsrch(void)
  * always makes the last line empty so this is good.
  */
  while (clp != (curbp->b_headp)) {
+ re_flags = REG_STARTEND;
+ if (tbo != 0)
+ re_flags |= REG_NOTBOL;
  regex_match[0].rm_so = tbo;
  regex_match[0].rm_eo = llength(clp);
  error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
-    RE_NMATCH, regex_match, REG_STARTEND);
+    RE_NMATCH, regex_match, re_flags);
  if (error != 0) {
  clp = lforw(clp);
  tdotline++;

Reply | Threaded
Open this post in threaded view
|

Re: mg: segmentation fault when using query-replace-regexp

Kenneth Westerback
On Tue, Jul 21, 2020 at 05:33:39PM +0200, Theo Buehler wrote:

> > Hmm. I get neither a seg fault nor a loop on my amd64 Lenove E595. The
> > cursor moves to different spots in X/i3 xterm vs console and in either
> > case doesn't do any replacement.
>
> I can reproduce both behaviors reliably as follows both on console and
> in xterm.
>
> $ echo 'a\n\n\nb' > /tmp/test
> $ mg /tmp/test
>
> Don't move the cursor, just do the following (what is between quotation
> marks is what you are supposed to type, the other bits are prompts):
>
> "M-x query-replace-regexp<enter>"

Aha. I was just doing "M-%" which is query-replace and not
query-replace-regexp as I thought.

> RE Query replace: "^<enter>"
> Query replace ^ with: "a<enter>"
> Query replacing ^ with a: "<enter>"
> <SP> replace, [.] rep-end, <DEL> don't, [!] repl rest <ESC> quit "!"

And now I get this.

>
> After hitting "!" here, it will enter a loop and eventually print
>
> panic: Out of memory in undo code (record)
>
> and exit.
>
> If I do the exact same dance after moving the cursor to the second
> (empty) line, it will segfault.
>
> With the diff below (which is what I understood to be Hiltjo's
> suggestion), I get more or less what I would expect. I think it should
> not replace the empty line after the last newline...
>
> So I think both diffs are not quite right :)

Well, I think not quite right is much better than looping or
segfault. So I'd go ahead with either diff at your discretion.

.... Ken

>
> Index: line.c
> ===================================================================
> RCS file: /var/cvs/src/usr.bin/mg/line.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 line.c
> --- line.c 29 Aug 2018 07:50:16 -0000 1.61
> +++ line.c 21 Jul 2020 14:58:49 -0000
> @@ -556,6 +556,8 @@ lreplace(RSIZE plen, char *st)
>   goto done;
>  
>   lp = curwp->w_dotp;
> + if (ltext(lp) == NULL)
> + goto done;
>   doto = curwp->w_doto;
>   n = plen;
>  
> Index: re_search.c
> ===================================================================
> RCS file: /var/cvs/src/usr.bin/mg/re_search.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 re_search.c
> --- re_search.c 9 Jul 2020 10:42:24 -0000 1.34
> +++ re_search.c 21 Jul 2020 15:00:08 -0000
> @@ -308,7 +308,7 @@ re_doreplace(RSIZE plen, char *st)
>  static int
>  re_forwsrch(void)
>  {
> - int tbo, tdotline, error;
> + int re_flags, tbo, tdotline, error;
>   struct line *clp;
>  
>   clp = curwp->w_dotp;
> @@ -330,10 +330,13 @@ re_forwsrch(void)
>   * always makes the last line empty so this is good.
>   */
>   while (clp != (curbp->b_headp)) {
> + re_flags = REG_STARTEND;
> + if (tbo != 0)
> + re_flags |= REG_NOTBOL;
>   regex_match[0].rm_so = tbo;
>   regex_match[0].rm_eo = llength(clp);
>   error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
> -    RE_NMATCH, regex_match, REG_STARTEND);
> +    RE_NMATCH, regex_match, re_flags);
>   if (error != 0) {
>   clp = lforw(clp);
>   tdotline++;

Reply | Threaded
Open this post in threaded view
|

Re: mg: segmentation fault when using query-replace-regexp

Theo Buehler-3
In reply to this post by Mark Willson-2
> Thanks very much for your review.  What you point out above is certainly
> true, but
> then it is consistent with the current behaviour when searching for the
> beginning-of-line ('^') anchor and the point at the beginning of a
> non-empty line. In that case, the point does not move.

This sounds like a convincing argument for my first version to me, so I
have committed that.  Thanks a lot to both of you.