sharplispers / clx

a fork of crhodes' fork of danb's fork of the CLX library, an X11 client for Common Lisp
Other
114 stars 46 forks source link

Type errors with sbcl 2.0 #160

Closed ghost closed 4 years ago

ghost commented 4 years ago

Apparently sbcl 2.0 is even more careful with types. This results in some old code getting an error with encode-wm-size-hints because some fields aren't properly converted from int32 to card32.

The old code is found in garnet. It's kind of hard to generate an example without saying "just build garnet and try some of the demos." The error shows up when negative numbers are passed in these fields.

But I have a fix:

--- manager.lisp~       2020-01-10 08:12:56.298651545 -0800
+++ manager.lisp        2020-01-11 11:28:08.193837006 -0800
@@ -392,8 +392,8 @@
     (when (and (wm-size-hints-x hints) (wm-size-hints-y hints)) 
       (unless (wm-size-hints-user-specified-position-p hints)
        (setf (ldb (byte 1 2) flags) 1))
-      (setf (aref vector 1) (wm-size-hints-x hints)
-           (aref vector 2) (wm-size-hints-y hints)))
+      (setf (aref vector 1) (int32->card32 (wm-size-hints-x hints))
+           (aref vector 2) (int32->card32 (wm-size-hints-y hints))))
     (when (and (wm-size-hints-width hints) (wm-size-hints-height hints))
       (unless (wm-size-hints-user-specified-size-p hints)
        (setf (ldb (byte 1 3) flags) 1))

I'm not sure there aren't other places where this problem arises but I haven't seen any so far.

ghost commented 4 years ago

Sorry, here's the patch attached to avoid mangling:

manager-patch.txt

dkochmanski commented 4 years ago

fixed, thanks!

dkochmanski commented 4 years ago

(I've accidentally pushed to master instead of making a pull request, but the change is small enough to not try to reverse it to resubmit a PR -- I've reindented the file in a second commit).

JMC-design commented 4 years ago

I haven't had time to look at this but is this the right place and correct way to fix this? The slot is defined to hold an int32 and there is a bit test in decode-wm-size-hints. Is it correct? Should there be a corresponding one in encode?

ghost commented 4 years ago

The code in decode-wm-size-hints checks to see if the flags wm-size-hints-user-specified-position-p and/or wm-size-hints-user-specified-size-p are set. If so, it pulls the data out of the buffer and puts it into the corresponding fields. This is obfuscated because those flags are encoded in the bits being checked. Note that in decode-wm-size-hints, the card32 is converted into an int32 for the x and y fields.

My fix simply does the reverse; in encode-wm-size-hints it converts the int32s into card32s before putting them in the buffer. This conversion was apparently omitted by mistake and no lisp implementation checked types closely enough to catch it.

I actually found what seems to be another error. (Should I open a separate issue for this?) The error doesn't seem to have any effect at present, but who knows what might happen later?

Anyway it's in the file macros.lisp, the macro decode-type. Here's the code:

(defmacro decode-type (type value)
  ;; Given an integer and type, return the value
  (let ((args nil))
    (when (consp type)
      (setq args (cdr type)
            type (car type)))
    `(macrolet ((read-card29 (value) value)
                (read-card32 (value) value)
                (read-int32 (value) `(card32->int32 ,value))
                (read-card16 (value) value)
                (read-int16 (value) `(card16->int16 ,value))
                (read-card8 (value) value)
                (read-int8 (value) `(int8->card8 ,value)))
       (,(getify type) ,value ,@args))))

This is obviously wrong. The line

(read-int8 (value) `(int8->card8 ,value)))

should be

(read-int8 (value) `(card8->int8 ,value)))

Apparently nothing to date cared about the difference between a card8 and an int8 but maybe at a later date it might matter?