clisp-devel

division by zero in new-clx

[Shawn B. <sa...@gm...>]

A user reports that mplayer 1.3 crashes stumpwm. The backtrace
revealed a problem in decode-wm-size-hints. Here's the call, taken
from that backtrace, that breaks new-clx.

(xlib::decode-wm-size-hints #(924 0 528 0 349 0 624 0 352 0 4 0 4 0 0 0 0 0))

It seems mplayer uses 0,0 for the max aspect ratio to mean that there
is no max aspect ratio.

-Shawn

Re: division by zero in new-clx

[Sam S. <sd...@gn...>]

Shawn Betts wrote:
> A user reports that mplayer 1.3 crashes stumpwm. The backtrace
> revealed a problem in decode-wm-size-hints. Here's the call, taken
> from that backtrace, that breaks new-clx.
> 
> (xlib::decode-wm-size-hints #(924 0 528 0 349 0 624 0 352 0 4 0 4 0 0 0 0 0))
> 
> It seems mplayer uses 0,0 for the max aspect ratio to mean that there
> is no max aspect ratio.

the offending code:

(defun decode-wm-size-hints (vector)
   (when vector
     (let ((flags (aref vector 0))
           (hints (make-wm-size-hints)))
....
       (when (logbitp 7 flags)
         (setf (wm-size-hints-min-aspect hints) (/ (aref vector 11) (aref vector
12))
               (wm-size-hints-max-aspect hints) (/ (aref vector 13) (aref vector
14))))
....


is exactly the same in all CLX implementations I can see.
basically, mplayer is buggy, if it does not want to specify the aspect ratio,
it should unset the 7th bit of the first vector element.
did you report this to the mplayer maintainers?

to work around the bug:

--- clx.lisp.~1.42.~	2008-12-08 09:16:31.000000000 -0500
+++ clx.lisp	2009-08-26 10:11:22.000041000 -0400
@@ -783,8 +783,12 @@
          (setf (wm-size-hints-width-inc hints) (aref vector 9)
                (wm-size-hints-height-inc hints) (aref vector 10)))
        (when (logbitp 7 flags)
-        (setf (wm-size-hints-min-aspect hints) (/ (aref vector 11) (aref
vector 12))
-              (wm-size-hints-max-aspect hints) (/ (aref vector 13) (aref
vector 14))))
+        (let ((low (aref vector 12)))
+          (unless (zerop low)
+            (setf (wm-size-hints-min-aspect hints) (/ (aref vector 11) low))))
+        (let ((low (aref vector 14)))
+          (unless (zerop low)
+            (setf (wm-size-hints-max-aspect hints) (/ (aref vector 13) low)))))
        (when (> (length vector) 15)
          ;; This test is for backwards compatibility since old Xlib programs
          ;; can set a size-hints structure that is too small.  See ICCCM.

I bet all the other WMs had to do something like that :-(

Sam

Re: division by zero in new-clx

[Sam S. <sd...@gn...>]

> * Shawn Betts <fn...@tz...> [2009-08-25 21:07:31 -0700]:
>
> A user reports that mplayer 1.3 crashes stumpwm. The backtrace
> revealed a problem in decode-wm-size-hints. Here's the call, taken
> from that backtrace, that breaks new-clx.
>
> (xlib::decode-wm-size-hints #(924 0 528 0 349 0 624 0 352 0 4 0 4 0 0 0 0 0))
>
> It seems mplayer uses 0,0 for the max aspect ratio to mean that there
> is no max aspect ratio.

the culprit is the clx code present in all clx implementations I can see.
IOW, this, I think, is a bug in mplayer: if they do not want to supply
the aspect ratio, they should unset the 7th bit in the first vector element.
I can, of course, add a workaround to this bug:

--- clx.lisp.~1.42.~	2008-12-08 00:01:13.000000000 -0500
+++ clx.lisp	2009-08-29 22:35:32.000000000 -0400
@@ -783,8 +783,12 @@
         (setf (wm-size-hints-width-inc hints) (aref vector 9)
               (wm-size-hints-height-inc hints) (aref vector 10)))
       (when (logbitp 7 flags)
-        (setf (wm-size-hints-min-aspect hints) (/ (aref vector 11) (aref vector 12))
-              (wm-size-hints-max-aspect hints) (/ (aref vector 13) (aref vector 14))))
+        (let ((d (aref vector 12)))
+          (unless (zerop d)
+            (setf (wm-size-hints-min-aspect hints) (/ (aref vector 11) d))))
+        (let ((d (aref vector 14)))
+          (unless (zerop d)
+            (setf (wm-size-hints-max-aspect hints) (/ (aref vector 13) d)))))
       (when (> (length vector) 15)
         ;; This test is for backwards compatibility since old Xlib programs
         ;; can set a size-hints structure that is too small.  See ICCCM.

however, this should be propagated to all the other CLX repositories...


-- 
Sam Steingold (http://sds.podval.org/) on Ubuntu 9.04 (jaunty)
http://www.PetitionOnline.com/tap12009/ http://honestreporting.com
http://palestinefacts.org http://iris.org.il http://jihadwatch.org
usually: can't pay ==> don't buy. software: can't buy ==> don't pay

Re: division by zero in new-clx

[Shawn B. <sa...@gm...>]

On Sat, Aug 29, 2009 at 7:37 PM, Sam Steingold<sd...@gn...> wrote:
>> * Shawn Betts <fn...@tz...> [2009-08-25 21:07:31 -0700]:
>>
>> A user reports that mplayer 1.3 crashes stumpwm. The backtrace
>> revealed a problem in decode-wm-size-hints. Here's the call, taken
>> from that backtrace, that breaks new-clx.
>>
>> (xlib::decode-wm-size-hints #(924 0 528 0 349 0 624 0 352 0 4 0 4 0 0 0 0 0))
>>
>> It seems mplayer uses 0,0 for the max aspect ratio to mean that there
>> is no max aspect ratio.
>
> the culprit is the clx code present in all clx implementations I can see.
> IOW, this, I think, is a bug in mplayer: if they do not want to supply
> the aspect ratio, they should unset the 7th bit in the first vector element.
> I can, of course, add a workaround to this bug:

They do specify a min aspect ratio. I reported this as a bug to clisp
because I think the clx library should be robust enough to detect
badly formed properties and deal with them appropriately. Crashing
because of badly formatted data from another client is Bad! haven't
sent a report to mplayer but I will. I'll let you know if they have
anything interesting to say.

-Shawn

Re: division by zero in new-clx

[Sam S. <sd...@gn...>]

Shawn Betts wrote:
> On Sat, Aug 29, 2009 at 7:37 PM, Sam Steingold<sd...@gn...> wrote:
>>> * Shawn Betts <fn...@tz...> [2009-08-25 21:07:31 -0700]:
>>>
>>> A user reports that mplayer 1.3 crashes stumpwm. The backtrace
>>> revealed a problem in decode-wm-size-hints. Here's the call, taken
>>> from that backtrace, that breaks new-clx.
>>>
>>> (xlib::decode-wm-size-hints #(924 0 528 0 349 0 624 0 352 0 4 0 4 0 0 0 0 0))
>>>
>>> It seems mplayer uses 0,0 for the max aspect ratio to mean that there
>>> is no max aspect ratio.
>> the culprit is the clx code present in all clx implementations I can see.
>> IOW, this, I think, is a bug in mplayer: if they do not want to supply
>> the aspect ratio, they should unset the 7th bit in the first vector element.
>> I can, of course, add a workaround to this bug:
> 
> They do specify a min aspect ratio. I reported this as a bug to clisp
> because I think the clx library should be robust enough to detect
> badly formed properties and deal with them appropriately. Crashing
> because of badly formatted data from another client is Bad! haven't

indeed. note that this bug is present in ALL clx implementations.
this means that we will have to work to propagate the fix everywhere if we are
to keep any code consistency...

> sent a report to mplayer but I will. I'll let you know if they have
> anything interesting to say.

thanks.

Re: division by zero in new-clx

[Sam S. <sd...@gn...>]

> * Shawn Betts <fn...@tz...> [2009-08-25 21:07:31 -0700]:
>
> A user reports that mplayer 1.3 crashes stumpwm. The backtrace
> revealed a problem in decode-wm-size-hints. Here's the call, taken
> from that backtrace, that breaks new-clx.
>
> (xlib::decode-wm-size-hints #(924 0 528 0 349 0 624 0 352 0 4 0 4 0 0 0 0 0))
>
> It seems mplayer uses 0,0 for the max aspect ratio to mean that there
> is no max aspect ratio.

cf bug https://sourceforge.net/tracker/?func=detail&atid=101355&aid=3292605&group_id=1355

I suspect that Ben Spencer is right and, indeed, we need to fix
XLIB:GET-PROPERTY.
Alas, the appended patch bombs with

bad gravity: 6587458

when I try to get window size hints for _all_ windows with

(xlib:with-open-display (dpy)
  (dolist (screen (xlib:display-roots dpy))
    (dolist (window (xlib:query-tree (xlib:screen-root screen)))
      (multiple-value-bind (data type format bytes-after)
          (xlib:get-property window :WM_NORMAL_HINTS)
        (when data
          (assert (eq type :WM_SIZE_HINTS))
          (assert (= format 32))
          (assert (= bytes-after 0))
          (assert (= (length data) 18))
          (print (list (xlib:wm-name window) data))
          (print (xlib:wm-normal-hints window)))))))

which means that somehow gravity is specified incorrectly.

Any insights?

-- 
Sam Steingold (http://sds.podval.org/) on CentOS release 5.6 (Final) X 11.0.60900031
http://memri.org http://truepeace.org http://iris.org.il
http://mideasttruth.com http://ffii.org http://thereligionofpeace.com
Genius is immortal, but morons live longer.


diff -r c0d8b51788b8 modules/clx/new-clx/clx.f
--- a/modules/clx/new-clx/clx.f	Tue Apr 26 00:21:41 2011 -0700
+++ b/modules/clx/new-clx/clx.f	Wed Apr 27 18:21:12 2011 -0400
@@ -415,6 +415,7 @@ For further for grepability I use the fo
 #include "clisp.h"
 #include <X11/Xlib.h>
 #include <X11/Xutil.h>          /* XGetVisualInfo */
+#include <X11/Xatom.h>
 #include <X11/Xcms.h>   /* forXcmsCCCOfColormap() & XcmsVisualOfCCC() */
 #include <X11/Xauth.h>
 /* #include <X11/Xresource.h> */
@@ -5509,10 +5510,15 @@ DEFUN(XLIB:GET-PROPERTY,window property 
         if (boundp(*transform_f))
           pushSTACK(*transform_f); /* transform function .. */
 
+        /* http://www.x.org/releases/X11R7.6/doc/man/man3/XChangeProperty.3.xhtml */
         switch (actual_format_return) {
-          case  8: pushSTACK(make_uint8(prop_return[i])); break;
-          case 16: pushSTACK(make_uint16(((uint16*)prop_return)[i])); break;
-          case 32: pushSTACK(make_uint32(((uint32*)prop_return)[i])); break;
+          case  8: /* the returned data is represented as a char array */
+            pushSTACK(fixnum(((char*)prop_return)[i])); break;
+          case 16: /* array of short int type and should be cast to that type */
+            pushSTACK(fixnum(((short*)prop_return)[i])); break;
+          case 32: /* array of longs (which in a 64-bit application will be
+                      64-bit values that are padded in the upper 4 bytes). */
+            pushSTACK(L_to_I(((long*)prop_return)[i])); break;
           default:
             NOTREACHED;
         }
@@ -5526,6 +5532,50 @@ DEFUN(XLIB:GET-PROPERTY,window property 
       pushSTACK(value1);
     }
 
+    if (actual_type_return == XA_WM_SIZE_HINTS) {
+        XSizeHints h;
+        long supplied_return = 0;
+        memset(&h,0,sizeof(h));
+        Status s = XGetWMSizeHints(display,w,&h,&supplied_return,property);
+        if (actual_format_return != 32) {
+            fprintf(stderr,"\nbad actual_format_return=%d\n",actual_format_return);
+            abort();
+        }
+        if (s == 0) {
+            fprintf(stderr,"\nXGetWMSizeHints failed\n");
+            abort();
+        }
+        if (nitems_return != 18) {
+            fprintf(stderr,"\nnitems_return=%d\n",nitems_return);
+            abort();
+        }
+        fprintf(stderr,"\nflags=%d  supplied_return=%d\n",h.flags,supplied_return); fflush(stderr);
+        ASSERT(h.flags == ((long*)prop_return)[0]);
+        ASSERT(h.x == ((long*)prop_return)[1]);
+        ASSERT(h.y == ((long*)prop_return)[2]);
+        ASSERT(h.width == ((long*)prop_return)[3]);
+        ASSERT(h.height == ((long*)prop_return)[4]);
+        ASSERT(h.min_width == ((long*)prop_return)[5]);
+        ASSERT(h.min_height == ((long*)prop_return)[6]);
+        ASSERT(h.max_width == ((long*)prop_return)[7]);
+        ASSERT(h.max_height == ((long*)prop_return)[8]);
+        ASSERT(h.width_inc == ((long*)prop_return)[9]);
+        ASSERT(h.height_inc == ((long*)prop_return)[10]);
+        ASSERT(h.min_aspect.x == ((long*)prop_return)[11]);
+        ASSERT(h.min_aspect.y == ((long*)prop_return)[12]);
+        ASSERT(h.max_aspect.x == ((long*)prop_return)[13]);
+        ASSERT(h.max_aspect.y == ((long*)prop_return)[14]);
+        ASSERT(h.base_width == ((long*)prop_return)[15]);
+        ASSERT(h.base_height == ((long*)prop_return)[16]);
+        ASSERT(h.win_gravity == ((long*)prop_return)[17]);
+        if (h.flags & PWinGravity
+            && (h.win_gravity > StaticGravity
+                || h.win_gravity < ForgetGravity)) {
+            fprintf(stderr,"\nbad gravity: %d\n",h.win_gravity);
+            abort();
+        }
+    }
+
     if (prop_return)
       X_CALL(XFree (prop_return));
 

Re: division by zero in new-clx

[Sam S. <sd...@gn...>]

> * Shawn Betts <fn...@tz...> [2009-08-25 21:07:31 -0700]:
>
> A user reports that mplayer 1.3 crashes stumpwm. The backtrace
> revealed a problem in decode-wm-size-hints. Here's the call, taken
> from that backtrace, that breaks new-clx.
>
> (xlib::decode-wm-size-hints #(924 0 528 0 349 0 624 0 352 0 4 0 4 0 0 0 0 0))
>
> It seems mplayer uses 0,0 for the max aspect ratio to mean that there
> is no max aspect ratio.

assuming this was a 64-bit machine, this bug should be fixed in mercurial.

-- 
Sam Steingold (http://sds.podval.org/) on CentOS release 5.6 (Final) X 11.0.60900031
http://thereligionofpeace.com http://www.memritv.org http://dhimmi.com
http://www.PetitionOnline.com/tap12009/ http://jihadwatch.org
Winners never quit; quitters never win; idiots neither win nor quit.