struct tail array, compare and segv

struct tail array, compare and segv

[Kevin Ryde]

Brought across from bug-guile ...

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> Actually, `scm_i_struct_equalp ()' should also compare the "tail
> elements" (when there are tail elements),

Is it as easy as using getting the size (of each) from
"scm_struct_i_n_words", instead of just using the layout?  I see
scm_struct_ref uses that for its range check.

> but their semantics are a
> little fuzzy to me.  In particular, I don't understand why the size of
> the tail array can be specified in both `make-vtable-vtable' and
> `make-struct': What does that mean?

Nosing around the code, I think make-vtable-vtable adds space to the
vtable (or rather vtable-vtable) struct like "class data".  Then
make-struct adds space to a struct created from it like "instance
data".

> Which one should really be taken into account?

Both :-).  But since both are structs I think both birds can be killed
with the one stone.

> It seems that the code is a bit unclear on this too:
>
>   guile> (define v (make-vtable-vtable "pr" 0))
>   guile> (define s (make-struct v 123))
>   guile> (struct-ref s 10)
>   Segmentation fault

Digging around I think that to have a tail array you're meant to say
"pR" or "pW" or whatever at the end of the layout.  Without R, W or O
then it looks like the internal func scm_struct_init doesn't
initialize the tail array contents, so then scm_struct_ref reads out
garbage as an SCM, and that can cause a segv if you attempt to print
it.

Should make-struct throw an error, or should it initialize?  I guess
initializing is the smallest change, but it looks like struct-set!
refuses to work in the tail part without "W" at the end, so maybe
R,W,O is how it's meant to be.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: struct tail array, compare and segv

[Ludovic Courtès]

Hi,

Kevin Ryde <user42@zip.com.au> writes:

>> but their semantics are a
>> little fuzzy to me.  In particular, I don't understand why the size of
>> the tail array can be specified in both `make-vtable-vtable' and
>> `make-struct': What does that mean?
>
> Nosing around the code, I think make-vtable-vtable adds space to the
> vtable (or rather vtable-vtable) struct like "class data".  Then
> make-struct adds space to a struct created from it like "instance
> data".

Hmm, right.

>> It seems that the code is a bit unclear on this too:
>>
>>   guile> (define v (make-vtable-vtable "pr" 0))
>>   guile> (define s (make-struct v 123))
>>   guile> (struct-ref s 10)
>>   Segmentation fault
>
> Digging around I think that to have a tail array you're meant to say
> "pR" or "pW" or whatever at the end of the layout.  Without R, W or O
> then it looks like the internal func scm_struct_init doesn't
> initialize the tail array contents, so then scm_struct_ref reads out
> garbage as an SCM, and that can cause a segv if you attempt to print
> it.

Yes, ok.

> Should make-struct throw an error, or should it initialize?  I guess
> initializing is the smallest change, but it looks like struct-set!
> refuses to work in the tail part without "W" at the end, so maybe
> R,W,O is how it's meant to be.

Yes, `make-struct' should check the vtable's layout last field, checking
whether it contains a capital letter, and throw an error if
TAIL_ARRAY_SIZE is non-zero and the last layout letter is not
capitalized.  Once this check is added, `struct-ref' can keep blindly
trusting the `n_words' thing.

The layout machinery is a bit heavyweight and ought to be factorized I
think.  Layout info could also be compiled to a more readily usable
form.

Thanks,
Ludovic.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: struct tail array, compare and segv

[Kevin Ryde]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

I'm looking at this to allow non-zero tail array only when the layout
provides for it.  I think all the internal uses of structs have no
tail array (and a zero size) so that should all be ok.  Dunno if
anyone else might have used the tail size to get some sneaky extra
space.  Sounds like the wrong thing to do, but could always be
loosened up again later.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: struct.c.tail.diff --]
[-- Type: text/x-diff, Size: 1045 bytes --]

--- struct.c.~1.111.2.4.~	2007-02-22 09:37:43.000000000 +1100
+++ struct.c	2007-02-26 10:40:36.000000000 +1100
@@ -430,6 +430,27 @@
   layout = SCM_PACK (SCM_STRUCT_DATA (vtable) [scm_vtable_index_layout]);
   basic_size = scm_i_symbol_length (layout) / 2;
   tail_elts = scm_to_size_t (tail_array_size);
+
+  /* A tail array is only allowed if the layout fields string ends in "R",
+     "W" or "O". */
+  if (tail_elts != 0)
+    {
+      SCM layout_str, last_char;
+      int last_c;
+      
+      if (basic_size == 0)
+        {
+        bad_tail_size: 
+          SCM_MISC_ERROR ("tail array not allowed unless layout ends R, W, or O", SCM_EOL);
+        }
+
+      layout_str = scm_symbol_to_string (layout);
+      last_char = scm_string_ref (layout_str,
+                                  scm_from_size_t (2 * basic_size - 1));
+      if (! SCM_LAYOUT_TAILP (SCM_CHAR (last_char)))
+        goto bad_tail_size;
+    }
+    
   SCM_CRITICAL_SECTION_START;
   if (SCM_STRUCT_DATA (vtable)[scm_struct_i_flags] & SCM_STRUCTF_ENTITY)
     {

[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: struct tail array, compare and segv

[Ludovic Courtès]

Hi,

Kevin Ryde <user42@zip.com.au> writes:

> I'm looking at this to allow non-zero tail array only when the layout
> provides for it.  I think all the internal uses of structs have no
> tail array (and a zero size) so that should all be ok.  Dunno if
> anyone else might have used the tail size to get some sneaky extra
> space.  Sounds like the wrong thing to do, but could always be
> loosened up again later.

Your patch looks good to me.

I think it is safe to assume than nobody has passed a non-zero tail size
to `make-struct' when the underlying vtable's layout doesn't allow for
it because the resulting struct is just unusable (its tail elements are
left uninitialized, as we saw earlier in one of these threads).

Thanks,
Ludovic.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: struct tail array, compare and segv

[Neil Jerram]

Kevin Ryde <user42@zip.com.au> writes:

> I'm looking at this to allow non-zero tail array only when the layout
> provides for it.  I think all the internal uses of structs have no
> tail array (and a zero size) so that should all be ok.  Dunno if
> anyone else might have used the tail size to get some sneaky extra
> space.  Sounds like the wrong thing to do, but could always be
> loosened up again later.

Looks good, except I'm not keen on the "goto bad_tail_size;".  It
would be marginally more friendly to have different error messages for
the two cases anyway.  (e.g. as below and "tail array not allowed if
struct has a zero length layout")

Regards,
        Neil

>
> --- struct.c.~1.111.2.4.~	2007-02-22 09:37:43.000000000 +1100
> +++ struct.c	2007-02-26 10:40:36.000000000 +1100
> @@ -430,6 +430,27 @@
>    layout = SCM_PACK (SCM_STRUCT_DATA (vtable) [scm_vtable_index_layout]);
>    basic_size = scm_i_symbol_length (layout) / 2;
>    tail_elts = scm_to_size_t (tail_array_size);
> +
> +  /* A tail array is only allowed if the layout fields string ends in "R",
> +     "W" or "O". */
> +  if (tail_elts != 0)
> +    {
> +      SCM layout_str, last_char;
> +      int last_c;
> +      
> +      if (basic_size == 0)
> +        {
> +        bad_tail_size: 
> +          SCM_MISC_ERROR ("tail array not allowed unless layout ends R, W, or O", SCM_EOL);
> +        }
> +
> +      layout_str = scm_symbol_to_string (layout);
> +      last_char = scm_string_ref (layout_str,
> +                                  scm_from_size_t (2 * basic_size - 1));
> +      if (! SCM_LAYOUT_TAILP (SCM_CHAR (last_char)))
> +        goto bad_tail_size;
> +    }
> +    
>    SCM_CRITICAL_SECTION_START;
>    if (SCM_STRUCT_DATA (vtable)[scm_struct_i_flags] & SCM_STRUCTF_ENTITY)
>      {



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: struct tail array, compare and segv

[Kevin Ryde]

Neil Jerram <neil@ossau.uklinux.net> writes:
>
> "tail array not allowed if struct has a zero length layout"

You don't think "unless ends in R,W,O" is enough to exclude an empty
layout?  The aim, slightly, is to hint at the solution within the
error message, for the benefit of those not 100% up with how it should
be.  But either is ok by me, if anyone feels strongly about what to
say.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: struct tail array, compare and segv

[Neil Jerram]

Kevin Ryde <user42@zip.com.au> writes:

> Neil Jerram <neil@ossau.uklinux.net> writes:
>>
>> "tail array not allowed if struct has a zero length layout"
>
> You don't think "unless ends in R,W,O" is enough to exclude an empty
> layout?  The aim, slightly, is to hint at the solution within the
> error message, for the benefit of those not 100% up with how it should
> be.  But either is ok by me, if anyone feels strongly about what to
> say.

No, on second thoughts, I agree that your original message is fine for
both cases.  I still don't like the goto, but it's not a big deal.

Regards,
     Neil



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel