egallesio / STklos

STklos Scheme
http://stklos.net
GNU General Public License v2.0
68 stars 17 forks source link

Avoid some crashes with `#p` #566

Closed jpellegrini closed 7 months ago

jpellegrini commented 12 months ago

Hi @egallesio !

I'm not sure this patch makes sense, but... Well, it avoids some crashes, and it's very simple (but perhaps there is a better way to handle this?)

When #p...... is used, STklos may crash if the number read does not represent a valid STklos SCM object.

This patch fixes it by changing read_address in read.c. When the number that is read ends in:

00: this is a pointer. we check if it's good with a function from Boehm GC.
01: this is an integer. whatever the other bits are, it's OK
10: this is a small object (currently characters only).
    we *check* if this is really a character, and signal an
    error it it's not
11: this is a small constant. we check if it's larger than the
    last known small constant, and also trigger an error when
    it's appropriate.

Before (in both cases STklos crashes):

stklos> #pa                    ;; 1010, ends in 10 but not char
Received a SIGSEGV signal.
stklos> #p1f       ;; tagged small constant, but too large

**** PANIC:
Bad small constant 31
ABORT.

After (now STklos won't crash):

stklos>  #pa
**** Error:
%read: bad small object address #pa
stklos> #p1f
**** Error:
%read: bad small constant address #p1f

The patch also fixes the address cases...

jpellegrini commented 12 months ago

Included some tests... :)

These are very interesting:

(test "address-of and read back, string"
      "xyz"
      (read-from-string (format "#p~x" (address-of "xyz"))))

(test "address-of and read back, date"
      #t
      (date? (read-from-string (format "#p~x" (address-of (current-date))))))
jpellegrini commented 12 months ago
(address-ref (address-of x)) => x

I'm not sure it's useful.

jpellegrini commented 12 months ago

Hmmmmm. The garbage collector actually does what we need for the address case!

/* Return a pointer to the base (lowest address) of an object given     */
/* a pointer to a location within the object.                           */
/* I.e., map an interior pointer to the corresponding base pointer.     */
/* Note that with debugging allocation, this returns a pointer to the   */
/* actual base of the object, i.e. the debug information, not to        */
/* the base of the user object.                                         */
/* Return 0 if displaced_pointer doesn't point to within a valid        */
/* object.                                                              */
/* Note that a deallocated object in the garbage collected heap         */
/* may be considered valid, even if it has been deallocated with        */
/* GC_free.                                                             */
GC_API void * GC_CALL GC_base(void * /* displaced_pointer */);

I'll update this PR later.

jpellegrini commented 12 months ago

Ok, done! :)

With this PR (unless I'm mistaken) #p is completely safe!

jpellegrini commented 12 months ago

I've added address-ref, for completeness. If it's too silly, I can remove it from the PR.

jpellegrini commented 12 months ago

This is really nice -- see the garbage collector in action:

stklos> (address-of (make-vector 1000))
140094260412416
stklos> (address-ref 140094260412416)
#(#void #void ... #void)
stklos> (gc)
stklos> (address-ref 140094260412416)
**** Error:
address-ref: bad object address 140094260412416
egallesio commented 7 months ago

That's great. I have applied your PR (changed the way addresses are controlled with GC_base to avoid some crashes): GC_base returns NULL if an ddress is not within a region allocated by the GC. But it also returns a non null value when we are inside an allocated region (it returns the start of the allocated region). Consequently, we consider that an address is valid if it is strictly equal to the value returned by GC_base.

stklos> (define s (cons 1 2))
;; s
stklos> (address-of s)
140390690973536
stklos> (address-ref 140390690973536)
(1 . 2)
stklos> (address-ref (+ 140390690973536 8)) ;; doesn't crash anymore

Thanks @jpellegrini for this PR.

jpellegrini commented 7 months ago

Great! :)

(dotimes (i  (expt 2 18))  (with-handler (lambda (a) -1) (eval-from-string (format "#p~a" i ))))

This finishes without errors or crashes now! :partying_face: