Errors in X - Build Date Mar 14 2020 01:48:26 UTC

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

Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Tom Murphy-7
Hi,

  I upgraded from the March 8th snapshot to March 14th one and
  when running various apps in X, X crashes and goes back to Xenodm
  login screen.

  1. Run xterm, then run tmux in it:
     xterm: fatal IO error 35 (Resource temporarily unavailable) or
     KillClient on X server ":0"

    X crashes back to Xenodm

  2. Run Firefox:
     Gdk-Message: 10:34:14.918: /usr/local/lib/firefox/firefox: Fatal
     IO error 35 (Resource temporarily unavailable) on X server :0.
     Gdk-Message: 10:34:14.961: firefox: Fatal IO error 9 (Bad file
     descriptor) on X server :0.
     Gdk-Message: 10:34:14.954: /usr/local/lib/firefox/firefox: Fatal
     IO error 35 (Resource temporarily unavailable) on X server :0.

  I downgraded to Build date: 1583972840 - Thu Mar 12 00:27:20 UTC 2020
  and that seems to have fixed the problem.

  I see some commits for the Intel display driver by jsg@ on the 13th
  March.  Not sure if related...

  Dmesg attached below.

  Thanks,
  Tom

OpenBSD 6.6-current (RAMDISK_CD) #49: Fri Mar 13 19:47:01 MDT 2020
    [hidden email]:/usr/src/sys/arch/amd64/compile/RAMDISK_CD
real mem = 17040445440 (16251MB)
avail mem = 16519979008 (15754MB)
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 3.0 @ 0x7b288000 (41 entries)
bios0: vendor American Megatrends Inc. version "1.05.07" date 09/29/2017
bios0: PC Specialist LTD N13xWU
acpi0 at bios0: ACPI 6.0
acpi0: tables DSDT FACP APIC FPDT FIDT MCFG SSDT SSDT HPET UEFI SSDT SSDT SSDT DBGP DBG2 DMAR BGRT ASF! WSMT
acpimadt0 at acpi0 addr 0xfee00000: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz, 1696.74 MHz, 06-8e-0a
cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES,MELTDOWN
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: apic clock running at 24MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE
cpu at mainbus0: not configured
cpu at mainbus0: not configured
cpu at mainbus0: not configured
cpu at mainbus0: not configured
cpu at mainbus0: not configured
cpu at mainbus0: not configured
cpu at mainbus0: not configured
ioapic0 at mainbus0: apid 2 pa 0xfec00000, version 20, 120 pins
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus -1 (PEG0)
acpiprt2 at acpi0: bus -1 (PEG1)
acpiprt3 at acpi0: bus -1 (PEG2)
acpiprt4 at acpi0: bus -1 (RP17)
acpiprt5 at acpi0: bus -1 (RP18)
acpiprt6 at acpi0: bus -1 (RP19)
acpiprt7 at acpi0: bus -1 (RP20)
acpiprt8 at acpi0: bus -1 (RP21)
acpiprt9 at acpi0: bus -1 (RP22)
acpiprt10 at acpi0: bus -1 (RP23)
acpiprt11 at acpi0: bus -1 (RP24)
acpiprt12 at acpi0: bus 1 (RP01)
acpiprt13 at acpi0: bus -1 (RP02)
acpiprt14 at acpi0: bus -1 (RP03)
acpiprt15 at acpi0: bus -1 (RP04)
acpiprt16 at acpi0: bus 58 (RP05)
acpiprt17 at acpi0: bus 59 (RP06)
acpiprt18 at acpi0: bus -1 (RP07)
acpiprt19 at acpi0: bus -1 (RP08)
acpiprt20 at acpi0: bus -1 (RP09)
acpiprt21 at acpi0: bus -1 (RP10)
acpiprt22 at acpi0: bus -1 (RP11)
acpiprt23 at acpi0: bus -1 (RP12)
acpiprt24 at acpi0: bus -1 (RP13)
acpiprt25 at acpi0: bus -1 (RP14)
acpiprt26 at acpi0: bus -1 (RP15)
acpiprt27 at acpi0: bus -1 (RP16)
acpiec0 at acpi0
acpicpu at acpi0 not configured
acpitz at acpi0 not configured
"PNP0A08" at acpi0 not configured
acpicmos0 at acpi0
"PNP0C14" at acpi0 not configured
"INT33A1" at acpi0 not configured
"PNPC000" at acpi0 not configured
"PNP0C0C" at acpi0 not configured
"PNP0C0E" at acpi0 not configured
"PNP0C0D" at acpi0 not configured
"ACPI0003" at acpi0 not configured
"PNP0C0A" at acpi0 not configured
"PNP0C14" at acpi0 not configured
cpu0: using Skylake AVX MDS workaround
pci0 at mainbus0 bus 0
pchb0 at pci0 dev 0 function 0 "Intel Core 8G Host" rev 0x08
"Intel UHD Graphics 620" rev 0x07 at pci0 dev 2 function 0 not configured
xhci0 at pci0 dev 20 function 0 "Intel 100 Series xHCI" rev 0x21: msi, xHCI 1.0
usb0 at xhci0: USB revision 3.0
uhub0 at usb0 configuration 1 interface 0 "Intel xHCI root hub" rev 3.00/1.00 addr 1
"Intel 100 Series Thermal" rev 0x21 at pci0 dev 20 function 2 not configured
"Intel 100 Series MEI" rev 0x21 at pci0 dev 22 function 0 not configured
ahci0 at pci0 dev 23 function 0 "Intel 100 Series AHCI" rev 0x21: msi, AHCI 1.3.1
ahci0: port 0: 6.0Gb/s
scsibus0 at ahci0: 32 targets
sd0 at scsibus0 targ 0 lun 0: <ATA, WDC WDS500G2B0A-, X611> naa.5001b448b66ac709
sd0: 476940MB, 512 bytes/sector, 976773168 sectors, thin
ppb0 at pci0 dev 28 function 0 "Intel 100 Series PCIE" rev 0xf1: msi
pci1 at ppb0 bus 1
ppb1 at pci0 dev 28 function 4 "Intel 100 Series PCIE" rev 0xf1: msi
pci2 at ppb1 bus 58
rtsx0 at pci2 dev 0 function 0 "Realtek RTL8411B Card Reader" rev 0x01: msi
sdmmc0 at rtsx0: 4-bit, dma
re0 at pci2 dev 0 function 1 "Realtek 8168" rev 0x12: RTL8411B (0x5c80), msi, address 80:fa:5b:4f:5b:fb
rgephy0 at re0 phy 7: RTL8251 PHY, rev. 0
ppb2 at pci0 dev 28 function 5 "Intel 100 Series PCIE" rev 0xf1: msi
pci3 at ppb2 bus 59
iwm0 at pci3 dev 0 function 0 "Intel Dual Band Wireless-AC 8265" rev 0x78, msi
"Intel 200 Series LPC" rev 0x21 at pci0 dev 31 function 0 not configured
"Intel 100 Series PMC" rev 0x21 at pci0 dev 31 function 2 not configured
"Intel 200 Series HD Audio" rev 0x21 at pci0 dev 31 function 3 not configured
"Intel 100 Series SMBus" rev 0x21 at pci0 dev 31 function 4 not configured
isa0 at mainbus0
pckbc0 at isa0 port 0x60/5 irq 1 irq 12
pckbd0 at pckbc0 (kbd slot)
wskbd0 at pckbd0: console keyboard
efifb0 at mainbus0: 1920x1080, 32bpp
wsdisplay0 at efifb0 mux 1: console (std, vt100 emulation), using wskbd0
"Chicony Electronics Co.,Ltd. Chicony USB2.0 Camera" rev 2.00/10.19 addr 2 at uhub0 port 4 not configured
"vendor 0x8087 product 0x0a2b" rev 2.00/0.10 addr 3 at uhub0 port 5 not configured
softraid0 at root
scsibus1 at softraid0: 256 targets
sd1 at scsibus1 targ 1 lun 0: <OPENBSD, SR CRYPTO, 006>
sd1: 461318MB, 512 bytes/sector, 944780799 sectors
root on rd0a swap on rd0b dump on rd0b
iwm0: could not read firmware iwm-8265-34 (error 2)
iwm0: failed to load init firmware
iwm0: hw rev 0x230, fw ver 34.0.1, address 00:21:6b:f3:e1:34

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Tom Murphy-7
Hi,

   I tested -current src with and without jsg@'s commits from:
   https://marc.info/?l=openbsd-cvs&m=158415441706312&w=2

   Both kernels work fine.

   It looks like the problem was introduced between the March 12 and
   the March 13/14th snapshot.

   So I don't think it's those commits, it's something to do with the
   snapshot.

   Thanks,
   Tom

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Tom Murphy-7
In reply to this post by Tom Murphy-7
Hi,

  I've narrowed it down.
  Further testing shows it's not kernel, but Xenocara. If I
  install xbase66.tgz from 12th March, it's fine. If I use the
  newest snapshot xbase66.tgz, X crashes.

  Thanks,
  Tom

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Theo Buehler-3
On Sat, Mar 14, 2020 at 02:01:23PM +0000, Tom Murphy wrote:
> Hi,
>
>   I've narrowed it down.
>   Further testing shows it's not kernel, but Xenocara. If I
>   install xbase66.tgz from 12th March, it's fine. If I use the
>   newest snapshot xbase66.tgz, X crashes.

There was only one xenocara commit in that window:
https://marc.info/?l=openbsd-cvs&m=158413257531932&w=2

I managed to reproduce a cwm crash once while playing around with the
set-title option in tmux, but have trouble doing so now.

>
>   Thanks,
>   Tom
>

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Tom Murphy-7
On Sat, Mar 14, 2020 at 03:53:58PM +0100, Theo Buehler wrote:

> On Sat, Mar 14, 2020 at 02:01:23PM +0000, Tom Murphy wrote:
> > Hi,
> >
> >   I've narrowed it down.
> >   Further testing shows it's not kernel, but Xenocara. If I
> >   install xbase66.tgz from 12th March, it's fine. If I use the
> >   newest snapshot xbase66.tgz, X crashes.
>
> There was only one xenocara commit in that window:
> https://marc.info/?l=openbsd-cvs&m=158413257531932&w=2
>
> I managed to reproduce a cwm crash once while playing around with the
> set-title option in tmux, but have trouble doing so now.

It looks like this line just under the variable declarations
in client_set_name makes it all blow up:

free(cc->name);

It's not being checked if cc->name is NULL.

Yes, I am using cwm as my window manager.

Thanks,
Tom

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Tim van der Molen-5
Tom Murphy (2020-03-14 15:24 +0000):

> On Sat, Mar 14, 2020 at 03:53:58PM +0100, Theo Buehler wrote:
> > On Sat, Mar 14, 2020 at 02:01:23PM +0000, Tom Murphy wrote:
> > > Hi,
> > >
> > >   I've narrowed it down.
> > >   Further testing shows it's not kernel, but Xenocara. If I
> > >   install xbase66.tgz from 12th March, it's fine. If I use the
> > >   newest snapshot xbase66.tgz, X crashes.
> >
> > There was only one xenocara commit in that window:
> > https://marc.info/?l=openbsd-cvs&m=158413257531932&w=2
> >
> > I managed to reproduce a cwm crash once while playing around with the
> > set-title option in tmux, but have trouble doing so now.
>
> It looks like this line just under the variable declarations
> in client_set_name makes it all blow up:
>
> free(cc->name);
>
> It's not being checked if cc->name is NULL.
>
> Yes, I am using cwm as my window manager.

Very sorry about that. I've backed out the commit for now.

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Otto Moerbeek
In reply to this post by Tom Murphy-7
On Sat, Mar 14, 2020 at 03:24:16PM +0000, Tom Murphy wrote:

> On Sat, Mar 14, 2020 at 03:53:58PM +0100, Theo Buehler wrote:
> > On Sat, Mar 14, 2020 at 02:01:23PM +0000, Tom Murphy wrote:
> > > Hi,
> > >
> > >   I've narrowed it down.
> > >   Further testing shows it's not kernel, but Xenocara. If I
> > >   install xbase66.tgz from 12th March, it's fine. If I use the
> > >   newest snapshot xbase66.tgz, X crashes.
> >
> > There was only one xenocara commit in that window:
> > https://marc.info/?l=openbsd-cvs&m=158413257531932&w=2
> >
> > I managed to reproduce a cwm crash once while playing around with the
> > set-title option in tmux, but have trouble doing so now.
>
> It looks like this line just under the variable declarations
> in client_set_name makes it all blow up:
>
> free(cc->name);
>
> It's not being checked if cc->name is NULL.

That is not a problem. free(3) accepts NULL.

Looking a the diff, I do see a use-after-free.

        -Otto
>
> Yes, I am using cwm as my window manager.
>
> Thanks,
> Tom
>

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Stefan Sperling-5
On Sat, Mar 14, 2020 at 05:36:46PM +0100, Otto Moerbeek wrote:
> Looking a the diff, I do see a use-after-free.

The problem is the use of TAILQ_FOREACH(wn, ...) instead of
TAILQ_FOREACH_SAFE(wn, ...) in combination with free(wn), correct?

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Tim van der Molen-5
Stefan Sperling (2020-03-14 18:12 +0100):
> On Sat, Mar 14, 2020 at 05:36:46PM +0100, Otto Moerbeek wrote:
> > Looking a the diff, I do see a use-after-free.
>
> The problem is the use of TAILQ_FOREACH(wn, ...) instead of
> TAILQ_FOREACH_SAFE(wn, ...) in combination with free(wn), correct?

Most likely, yes.

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Okan Demirmen
On Sat 2020.03.14 at 18:33 +0100, Tim van der Molen wrote:
> Stefan Sperling (2020-03-14 18:12 +0100):
> > On Sat, Mar 14, 2020 at 05:36:46PM +0100, Otto Moerbeek wrote:
> > > Looking a the diff, I do see a use-after-free.
> >
> > The problem is the use of TAILQ_FOREACH(wn, ...) instead of
> > TAILQ_FOREACH_SAFE(wn, ...) in combination with free(wn), correct?
>
> Most likely, yes.

Revised diff using TAILQ_FOREACH_SAFE.

Index: client.c
===================================================================
RCS file: /home/open/cvs/xenocara/app/cwm/client.c,v
retrieving revision 1.260
diff -u -p -r1.260 client.c
--- client.c 14 Mar 2020 16:11:09 -0000 1.260
+++ client.c 14 Mar 2020 18:36:49 -0000
@@ -667,22 +667,24 @@ client_close(struct client_ctx *cc)
 void
 client_set_name(struct client_ctx *cc)
 {
- struct winname *wn;
- char *newname;
+ struct winname *wn, *wmnxt;
  int i = 0;
 
- if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &newname))
- if (!xu_get_strprop(cc->win, XA_WM_NAME, &newname))
- newname = xstrdup("");
+ free(cc->name);
+ if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &cc->name))
+ if (!xu_get_strprop(cc->win, XA_WM_NAME, &cc->name))
+ cc->name = xstrdup("");
 
- TAILQ_FOREACH(wn, &cc->nameq, entry) {
- if (strcmp(wn->name, newname) == 0)
+ TAILQ_FOREACH_SAFE(wn, &cc->nameq, entry, wmnxt) {
+ if (strcmp(wn->name, cc->name) == 0) {
  TAILQ_REMOVE(&cc->nameq, wn, entry);
+ free(wn->name);
+ free(wn);
+ }
  i++;
  }
- cc->name = newname;
  wn = xmalloc(sizeof(*wn));
- wn->name = xstrdup(newname);
+ wn->name = xstrdup(cc->name);
  TAILQ_INSERT_TAIL(&cc->nameq, wn, entry);
 
  /* Garbage collection. */

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Otto Moerbeek
On Sun, Mar 15, 2020 at 10:09:53AM -0400, Okan Demirmen wrote:

> On Sat 2020.03.14 at 18:33 +0100, Tim van der Molen wrote:
> > Stefan Sperling (2020-03-14 18:12 +0100):
> > > On Sat, Mar 14, 2020 at 05:36:46PM +0100, Otto Moerbeek wrote:
> > > > Looking a the diff, I do see a use-after-free.
> > >
> > > The problem is the use of TAILQ_FOREACH(wn, ...) instead of
> > > TAILQ_FOREACH_SAFE(wn, ...) in combination with free(wn), correct?
> >
> > Most likely, yes.
>
> Revised diff using TAILQ_FOREACH_SAFE.

Doesn't seem right.

>
> Index: client.c
> ===================================================================
> RCS file: /home/open/cvs/xenocara/app/cwm/client.c,v
> retrieving revision 1.260
> diff -u -p -r1.260 client.c
> --- client.c 14 Mar 2020 16:11:09 -0000 1.260
> +++ client.c 14 Mar 2020 18:36:49 -0000
> @@ -667,22 +667,24 @@ client_close(struct client_ctx *cc)
>  void
>  client_set_name(struct client_ctx *cc)
>  {
> - struct winname *wn;
> - char *newname;
> + struct winname *wn, *wmnxt;
>   int i = 0;
>  
> - if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &newname))
> - if (!xu_get_strprop(cc->win, XA_WM_NAME, &newname))
> - newname = xstrdup("");
> + free(cc->name);

cc->name is freed

> + if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &cc->name))
> + if (!xu_get_strprop(cc->win, XA_WM_NAME, &cc->name))
> + cc->name = xstrdup("");

And only re-assigned on certain conditions

>  
> - TAILQ_FOREACH(wn, &cc->nameq, entry) {
> - if (strcmp(wn->name, newname) == 0)
> + TAILQ_FOREACH_SAFE(wn, &cc->nameq, entry, wmnxt) {
> + if (strcmp(wn->name, cc->name) == 0) {
>   TAILQ_REMOVE(&cc->nameq, wn, entry);
> + free(wn->name);
> + free(wn);
> + }
>   i++;
>   }
> - cc->name = newname;
>   wn = xmalloc(sizeof(*wn));
> - wn->name = xstrdup(newname);
> + wn->name = xstrdup(cc->name);

and then used unconditioanlly

>   TAILQ_INSERT_TAIL(&cc->nameq, wn, entry);
>  
>   /* Garbage collection. */

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Okan Demirmen
On Sun 2020.03.15 at 15:45 +0100, Otto Moerbeek wrote:

> On Sun, Mar 15, 2020 at 10:09:53AM -0400, Okan Demirmen wrote:
>
> > On Sat 2020.03.14 at 18:33 +0100, Tim van der Molen wrote:
> > > Stefan Sperling (2020-03-14 18:12 +0100):
> > > > On Sat, Mar 14, 2020 at 05:36:46PM +0100, Otto Moerbeek wrote:
> > > > > Looking a the diff, I do see a use-after-free.
> > > >
> > > > The problem is the use of TAILQ_FOREACH(wn, ...) instead of
> > > > TAILQ_FOREACH_SAFE(wn, ...) in combination with free(wn), correct?
> > >
> > > Most likely, yes.
> >
> > Revised diff using TAILQ_FOREACH_SAFE.
>
> Doesn't seem right.
>
> >
> > Index: client.c
> > ===================================================================
> > RCS file: /home/open/cvs/xenocara/app/cwm/client.c,v
> > retrieving revision 1.260
> > diff -u -p -r1.260 client.c
> > --- client.c 14 Mar 2020 16:11:09 -0000 1.260
> > +++ client.c 14 Mar 2020 18:36:49 -0000
> > @@ -667,22 +667,24 @@ client_close(struct client_ctx *cc)
> >  void
> >  client_set_name(struct client_ctx *cc)
> >  {
> > - struct winname *wn;
> > - char *newname;
> > + struct winname *wn, *wmnxt;
> >   int i = 0;
> >  
> > - if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &newname))
> > - if (!xu_get_strprop(cc->win, XA_WM_NAME, &newname))
> > - newname = xstrdup("");
> > + free(cc->name);
>
> cc->name is freed
>
> > + if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &cc->name))
> > + if (!xu_get_strprop(cc->win, XA_WM_NAME, &cc->name))
> > + cc->name = xstrdup("");
>
> And only re-assigned on certain conditions

First it tries to get the name from _NET_WM_NAME Atom, then XA_WM_NAME
Atom, then lastly it gets set to "". xu_get_strprop() returns => 1 on
success and 0 when there is no value to assign. So cc->name should
always be assigned at this point.

> >  
> > - TAILQ_FOREACH(wn, &cc->nameq, entry) {
> > - if (strcmp(wn->name, newname) == 0)
> > + TAILQ_FOREACH_SAFE(wn, &cc->nameq, entry, wmnxt) {
> > + if (strcmp(wn->name, cc->name) == 0) {
> >   TAILQ_REMOVE(&cc->nameq, wn, entry);
> > + free(wn->name);
> > + free(wn);
> > + }
> >   i++;
> >   }
> > - cc->name = newname;
> >   wn = xmalloc(sizeof(*wn));
> > - wn->name = xstrdup(newname);
> > + wn->name = xstrdup(cc->name);
>
> and then used unconditioanlly
>
> >   TAILQ_INSERT_TAIL(&cc->nameq, wn, entry);
> >  
> >   /* Garbage collection. */
>

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Otto Moerbeek
On Sun, Mar 15, 2020 at 11:14:06AM -0400, Okan Demirmen wrote:

> On Sun 2020.03.15 at 15:45 +0100, Otto Moerbeek wrote:
> > On Sun, Mar 15, 2020 at 10:09:53AM -0400, Okan Demirmen wrote:
> >
> > > On Sat 2020.03.14 at 18:33 +0100, Tim van der Molen wrote:
> > > > Stefan Sperling (2020-03-14 18:12 +0100):
> > > > > On Sat, Mar 14, 2020 at 05:36:46PM +0100, Otto Moerbeek wrote:
> > > > > > Looking a the diff, I do see a use-after-free.
> > > > >
> > > > > The problem is the use of TAILQ_FOREACH(wn, ...) instead of
> > > > > TAILQ_FOREACH_SAFE(wn, ...) in combination with free(wn), correct?
> > > >
> > > > Most likely, yes.
> > >
> > > Revised diff using TAILQ_FOREACH_SAFE.
> >
> > Doesn't seem right.
> >
> > >
> > > Index: client.c
> > > ===================================================================
> > > RCS file: /home/open/cvs/xenocara/app/cwm/client.c,v
> > > retrieving revision 1.260
> > > diff -u -p -r1.260 client.c
> > > --- client.c 14 Mar 2020 16:11:09 -0000 1.260
> > > +++ client.c 14 Mar 2020 18:36:49 -0000
> > > @@ -667,22 +667,24 @@ client_close(struct client_ctx *cc)
> > >  void
> > >  client_set_name(struct client_ctx *cc)
> > >  {
> > > - struct winname *wn;
> > > - char *newname;
> > > + struct winname *wn, *wmnxt;
> > >   int i = 0;
> > >  
> > > - if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &newname))
> > > - if (!xu_get_strprop(cc->win, XA_WM_NAME, &newname))
> > > - newname = xstrdup("");
> > > + free(cc->name);
> >
> > cc->name is freed
> >
> > > + if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &cc->name))
> > > + if (!xu_get_strprop(cc->win, XA_WM_NAME, &cc->name))
> > > + cc->name = xstrdup("");
> >
> > And only re-assigned on certain conditions
>
> First it tries to get the name from _NET_WM_NAME Atom, then XA_WM_NAME
> Atom, then lastly it gets set to "". xu_get_strprop() returns => 1 on
> success and 0 when there is no value to assign. So cc->name should
> always be assigned at this point.

Right, thanks for the explanation.

        -Otto

>
> > >  
> > > - TAILQ_FOREACH(wn, &cc->nameq, entry) {
> > > - if (strcmp(wn->name, newname) == 0)
> > > + TAILQ_FOREACH_SAFE(wn, &cc->nameq, entry, wmnxt) {
> > > + if (strcmp(wn->name, cc->name) == 0) {
> > >   TAILQ_REMOVE(&cc->nameq, wn, entry);
> > > + free(wn->name);
> > > + free(wn);
> > > + }
> > >   i++;
> > >   }
> > > - cc->name = newname;
> > >   wn = xmalloc(sizeof(*wn));
> > > - wn->name = xstrdup(newname);
> > > + wn->name = xstrdup(cc->name);
> >
> > and then used unconditioanlly
> >
> > >   TAILQ_INSERT_TAIL(&cc->nameq, wn, entry);
> > >  
> > >   /* Garbage collection. */
> >

Reply | Threaded
Open this post in threaded view
|

Re: Errors in X - Build Date Mar 14 2020 01:48:26 UTC

Tim van der Molen-5
In reply to this post by Okan Demirmen
Okan Demirmen (2020-03-15 10:09 -0400):

> On Sat 2020.03.14 at 18:33 +0100, Tim van der Molen wrote:
> > Stefan Sperling (2020-03-14 18:12 +0100):
> > > On Sat, Mar 14, 2020 at 05:36:46PM +0100, Otto Moerbeek wrote:
> > > > Looking a the diff, I do see a use-after-free.
> > >
> > > The problem is the use of TAILQ_FOREACH(wn, ...) instead of
> > > TAILQ_FOREACH_SAFE(wn, ...) in combination with free(wn), correct?
> >
> > Most likely, yes.
>
> Revised diff using TAILQ_FOREACH_SAFE.

OK. Thanks.