Bugs caused by internally constructed objects

[camoy]

Some time ago I did a deep dive into the racket/draw contracts. Here is an observation that led me to some bugs: class contracts do not provide a guarantee on objects that are created from within the contract boundary.

Example 1

The numbers given to in-region? are expected to be real?. If you give imaginary numbers

(define reg (new region%))
(send reg in-region? 0+i 0+i)

the correct error from the region% class contract is raised:

; in-region?: contract violation
;   expected: real?
;   given: 0+1i
;   in: the 1st argument of
;       the in-region? method in
;       region%/c

However, regions can be created indirectly via the clipping region

(define r-dc (new record-dc%))
(send r-dc set-clipping-rect 0 0 550 400)
(define reg (send r-dc get-clipping-region))
(send reg in-region? 0+i 0+i)

and such will give an internal error since the region was not created through the protected region% class:

; cairo_in_fill: given value does not fit primitive C type
;   C type: _double*
;   value: 0.0+1.0i

Example 2

The fifth argument to get-argb-pixels is expected to be bytes?

(define bmp (make-bitmap 550 400))
(send bmp get-argb-pixels 0 0 0 0 #f)

and gives an error as such

; get-argb-pixels: contract violation
;   expected: bytes?
;   given: #f
;   in: an and/c case of
;       the 5th argument of
;       the get-argb-pixels method in
;       the range of ...

But bitmaps can be created through the make-bitmap method of canvases

(define frame (new frame% [label "Example"]))
(define canvas (new canvas% [parent frame]))
(define bmp (send canvas make-bitmap 550 400))
(send bmp get-argb-pixels 0 0 0 0 #f)

and in such a case an internal error is raised because it's unprotected

; bytes-length: contract violation
;   expected: bytes?
;   given: #f

Example 3

The blink-caret method of snips expects a dc<%>

(define sn (new snip%))
(send sn blink-caret #f #f #f)

and the contract gives this error

; blink-caret: contract violation
;   expected: (is-a?/c dc<%>)
;   given: #f
;   in: the 1st argument of
;       the blink-caret method in
;       the 2nd conjunct of ...

However, cloning the snip constructs a new object that is not protected

(define sn (new snip%))
(define sn2 (send sn copy))
(send sn2 blink-caret #f #f #f)

and gives this different error

; blink-caret method of snip%: expected argument of type <dc<%> instance>; given: #f; other arguments: #f #f

which doesn't come from a contract check, but is a (usually redundant) defensive check that is present in the blink-caret method.

Summary

These are just a few instances I found in racket/draw—there are probably quite a few more. I'm not sure what the takeaway is. My thought is that using (is-a?/c my-class%) and then subsequently assuming that objects satisfy the class contract on my-class% is dangerous since it assumes that my-class% objects are only constructed through the protected constructor. Using object/c contracts everywhere instead of is-a?/c isn't a panacea either because, as far as I know, they will not get collapsed and wrappers might quickly accumulate. So while these bugs that I mentioned can be fixed individually, I feel as though there is a deeper concern here with respect to class contracts in general.

[rfindler]

For the first example, it seems like the right fix is to add this:

[get-clipping-region (->m (instanceof/c region%/c))]

to the definition of record-dc%/c? (You also have to reorder the definitions to avoid use-before-definition errors here.)

You mention a concern about potential failures of collapsing: did you observe that kind of problem or is that more of an abstract worry?

[camoy]

No, I didn't observe a problem with the lack of collapsing it was just a speculation.

[rfindler]

It does seem like a potential problem but it isn't clear that these specific methods are more likely to be problems than the already existing set. A problem I've struggled more with along these lines has to do with replacing eq? checks with object=? checks. FWIW.

[rfindler]

Flipping through the contract files, I really do see a lot of is-a?/cs! Hm. I wonder if this was a systematic plan on @takikawa 's part and I've forgotten part of the story here.

[rfindler]

I wrote a script that tried to look for more opportunities for these kinds of bugs. I didn't turn up anything; I've just been looking at the graph via dot to see if anything jumps out at me.

The basic idea is that each node corresponds to objects of a certain class, in four variations: either the object is protected with a wrapper contract (or not) and the object is an input to or output from a protected method. Nodes get links between them based on the content of the contracts. For example, check out the make-pen/c contract. It has a method get-stipple that returns a bitmap. So, we're going to get an edge from one of the four pen nodes to one of the four bitmap nodes. In this case, because the bitmap is a result of the method, we'll get links from the positive to the positive and from the negative to the negative node. Also, because the contract on the bitmap is an is-a? contract and not bitmap%/c, we'll get a link from the wrapped to the unwrapped object.

Generally speaking, if we see a path from a positive protected object to a positive unprotected one, they we have a potential for an unwrapped value to be leaking out. These may be false positives, however, because, as in the example with the get-stipple method, it doesn't seem to be possible to get a pen to make a new stipple bitmap; it only gives you back stipple bitmaps that it got from you earlier, meaning it isn't a source of new bitmaps, only already-wrapped ones.

[mfelleisen]

(Boy a really good Spidey would do wonders here.)

[rfindler]

Matthias writes

(Boy a really good Spidey would do wonders here.)

The connection to static analysis is a nice way to think about this!

In particular, we could simply just replace all of is-a? contracts with instanceof/c contracts and feel secure in the fact that the checks will always happen. But this means that there will be lots of wrapping (not sure where it lives wrt to the "too much" line) and we'll want to make sure that the collapsing always happens. Also, there are probably some eq? tests hiding in there still (perhaps in hash tables) that should be object=? and those can be different to track down.

In other words, we can move the static analysis problem to a dynamic checking problem, but with a different "style" of failure than the usual tradeoffs with static analysis.

[rfindler]

The script found one more:

#lang racket
(require racket/draw)
(define bmp1 (make-bitmap 1 1))
(define tmp (make-temporary-file))
(define clr (make-object color% 244 200 200))
(define bmp2
  (dynamic-wind
   void
   (λ ()
     (send bmp1 save-file tmp 'bmp)
     (read-bitmap tmp
                  'unknown/alpha
                  clr
                  #:save-data-from-file? #t))
   (λ ()
     (delete-file tmp))))

(define-syntax-rule
  (show-err e)
  (with-handlers ([exn:fail? (λ (x) ((error-display-handler) (exn-message x) x))])
    e))

;; contract-based error message
(show-err (send (make-object color% 1 2 3) copy-from "wrong"))

;; checks inside color%
(show-err (send (vector-ref (send bmp2 get-data-from-file) 1) copy-from "wrong"))
(show-err (send (vector-ref (send bmp2 get-data-from-file) 1) set "wrong" "wrong" "wrong"))

(Kind of convoluted, tho.)

[rfindler]

Pen and brush lists are an interesting one, as it seems like there might be a way that some code would rely on eq? results from multiple calls to the find-or-create-... methods, but they won't be if we make this change to get contract error messages for the errors below.

#lang racket
(require racket/draw)

(define-syntax-rule
  (show-err e)
  (with-handlers ([exn:fail? (λ (x) ((error-display-handler) (exn-message x) x))])
    e))

(show-err (send (make-object pen%) set-width "wrong"))
(show-err (send (send the-pen-list find-or-create-pen "black" 1 'solid) set-width "wrong"))

(show-err (send (make-object brush%) set-color (vector "wrong")))
(show-err (send (send the-brush-list find-or-create-brush "black" 'solid) set-color (vector "wrong")))

[mfelleisen]

I see you have been at work. Nice!

It strikes me that pens and brushes can be checked for eq? ness in somewhat slower ways. I have strong doubts that this would affect performance all that much but yes, it would mean internal changes -- and hopefully no external ones.