Re: Xserver crash with cgoban

[Matthieu Herrb]

On Sat, Nov 05, 2022 at 08:55:46PM +0100, bau...@pestilenz.org wrote:

Hi,

Thanks for the report. You're right that this was overlooked when I
wrote this code.

I'd suggest the more complete (and paranoid) patch below:


Index: os/client.c
===================================================================
RCS file: /cvs/OpenBSD/xenocara/xserver/os/client.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 client.c
--- os/client.c 11 Nov 2021 09:03:14 -0000      1.5
+++ os/client.c 6 Nov 2022 10:41:04 -0000
@@ -160,20 +160,28 @@ DetermineClientCmd(pid_t pid, const char
         if (n != 1)
             return;
         argv = kvm_getargv(kd, kp, 0);
-        *cmdname = strdup(argv[0]);
-        i = 1;
-        while (argv[i] != NULL) {
-            len += strlen(argv[i]) + 1;
-            i++;
-        }
-        *cmdargs = calloc(1, len);
-        i = 1;
-        while (argv[i] != NULL) {
-            strlcat(*cmdargs, argv[i], len);
-            strlcat(*cmdargs, " ", len);
-            i++;
-        }
-        kvm_close(kd);
+       if (cmdname) {
+               if (argv == NULL || argv[0] == NULL) {
+                       *cmdname = ""; 
+                       return;
+               } else 
+                       *cmdname = strdup(argv[0]);
+       }
+       if (cmdargs) {
+               i = 1;
+               while (argv[i] != NULL) {
+                       len += strlen(argv[i]) + 1;
+                       i++;
+               }
+               *cmdargs = calloc(1, len);
+               i = 1;
+               while (argv[i] != NULL) {
+                       strlcat(*(char **)cmdargs, argv[i], len);
+                       strlcat(*(char **)cmdargs, " ", len);
+                       i++;
+               }
+       }
+       kvm_close(kd);
     }
 #else                           /* Linux using /proc/pid/cmdline */
 

> >Synopsis:    programs can crash xenocara servers
> >Category:    system
> >Environment:
>       System      : OpenBSD 7.1
>       Details     : OpenBSD 7.1-current (GENERIC.MP) #20: Thu Jul  7 13:04:50 
> CEST 2022
>                        
> bau...@x220.my.domain:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> 
>       Architecture: OpenBSD.amd64
>       Machine     : amd64
> >Description:
>       X server(s) crash when certain programs run (e.g. cgoban-1.9.12p3)
> >How-To-Repeat:
> 
>       % doas pkg_add cgoban
>       % cgoban
> 
> 
>       If you want to keep your X Session:
> 
>       % Xephyr :1 &
>       % env DISPLAY=:1 cgoban
> 
> >Fix:
>       As the manpage kvm_getargv(3) explains, programs may put
>       anything in their argv. And cgoban seems to set *argv to
>       NULL. If xprop(1) is run on cgoban after patching xenocara with the
>       diff below, xprop shows 
>          WM_COMMAND(STRING) = {  }
>     which supports my guess.
> 
>       
> Index: xserver/os/client.c
> ===================================================================
> RCS file: /cvs/xenocara/xserver/os/client.c,v
> retrieving revision 1.5
> diff -r1.5 client.c
> 163c163,170
> <         *cmdname = strdup(argv[0]);
> ---
> >         /* 
> >          * argv    may be NULL (error from kvm_getargv(3))
> >          * argv[0] may be NULL (cmd withheld by application)
> >          */
> >         if (argv == NULL || argv[0] == NULL) 
> >             *cmdname = strdup("");
> >         else
> >             *cmdname = strdup(argv[0]);
> 

-- 
Matthieu Herrb