[bug #25525] Segfault using goops

[bug #25525] Segfault using goops

[Michael Burschik]

URL:
  <http://savannah.gnu.org/bugs/?25525>

                 Summary: Segfault using goops
                 Project: Guile
            Submitted by: burschik
            Submitted on: Mon 09 Feb 2009 10:25:50 AM EET
                Category: None
                Severity: 3 - Normal
              Item Group: None
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any

    _______________________________________________________

Details:

(use-modules (oop goops))


(define-class <test-class> (<class>)
 name)


(make <test-class>)




This makes guile 1.8.5 crash with a segmentation fault on Linux.




    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?25525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #25525] Segfault using goops

[Neil Jerram]

Follow-up Comment #1, bug #25525 (project guile):

In case it helps until this is fixed properly...

This appears to be specific to the combination of
- the use of "name" in the definition of <test-class>
- the use of <class> as a superclass.

I would guess related to the fact that <class> already has a slot called
"name".


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?25525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #25525] Segfault using goops

[Stefan Israelsson Tampe]

Follow-up Comment #2, bug #25525 (project guile):


The reason is that code assume cpl to be a null terminated list but it's not
due to the bug, then a cdr will make the crash. Now this is a symptom and not
the cause. But we can avoid a segmentation fault by making sure that we check
for a pair although it will slow down the algorithm a little. Checking for a
pairs and silently move on if error yield an error in a layout check!

Better to fix the cause, right? Well to raise a question. by coding for
efficiency and assume that the code will be used correctly we will gain speed.
On the other hand this means that 
any bug will, with maybe to high probability, crash the repl.

I would prefere to use fast and unsecure paths only for proven
datastructures. Or, next best, use checks that is on only under a debug
compile!

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?25525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #25525] Segfault using goops

[Stefan Israelsson Tampe]

Follow-up Comment #3, bug #25525 (project guile):

a cut and paste bug, the function is
build_slots_list in goops.c

Anyhow I saw that I can enable a pair test, with a debug option in the
compilation.

So, as always, people are smart.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?25525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #25525] Segfault using goops

[Andy Wingo]

Follow-up Comment #4, bug #25525 (project guile):

This bug is still present in git.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?25525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #25525] Segfault using goops

[Stefan Israelsson Tampe]

Follow-up Comment #5, bug #25525 (project guile):

Ok, I did an attempt to make an acceptable fix to this.

The problem was that in <class> the name slot order of the layout
changed due to the new slot again called name. This cassed lookup code that
assumes a fixed layout to fetch the wrong values due to a shift of the layout.
The added fix will, at class creation, verify if the class is deducing
<class>. If so and there is an attempt to add a new slot named already defined
by <class>, an error will be signaled, see result below,

scheme@(guile-user)> (define-class <test-class> (<class>) name)
ERROR: In procedure %compute-slots:
ERROR: In procedure init-object: a predefined <class> inherrited field cannot
be redefined

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
scheme@(guile-user) [1]> 

A patch includeing some solidification of the code by making sure
a pair is used where it is assumed it does is added as well.

See goopsdiff diff below.

(file #23840)
    _______________________________________________________

Additional Item Attachment:

File name: goopsdiff.diff                 Size:4 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?25525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #25525] Segfault using goops

[Stefan Israelsson Tampe]

Follow-up Comment #6, bug #25525 (project guile):

I recognized that the layout problem appears as well if you inherit
from first <class> and then a class <b> which does not contain <class> I did
fix this. By postponing the addition of <class> variables to the last in order
for them to always get the correct
index in the layout. 

So, now 
(1) you can inheret meta objects from both <class> and non <class> objects

(2) it's not possible to inherit or add variables that have the same name as
<class> variables. If that is tried an error is signaled.

For a git-diff, see goopsdiff2 below

(file #23843)
    _______________________________________________________

Additional Item Attachment:

File name: goopsdiff2.diff                Size:4 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?25525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

[bug #25525] Segfault using goops

[Stefan Israelsson Tampe]

Follow-up Comment #7, bug #25525 (project guile):

added goops4.diff, a git diff that fixes c stylish errors and use
scm_is_eq in stead of a direct comparision.

/Stefan

(file #23943)
    _______________________________________________________

Additional Item Attachment:

File name: goops4.diff                    Size:3 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?25525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

bug#9815: [bug #25525] Segfault using goops

[Andy Wingo]

Update of bug #25525 (project guile):

                  Status:                    None => Fixed                  
             Open/Closed:                    Open => Closed                 

    _______________________________________________________

Follow-up Comment #8:

Thank you for the fix, Stefan.  I have pushed it to git along with a test
case.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?25525>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/

Re: [bug #25525] Segfault using goops

[Andy Wingo]

Hi Stefan,

I appreciate very much your work on the bugs!  Your fixes have been
thoughtful and mostly correct, it seems to me.

In this case, your approach sounds about right.  There are a number of
style errors that need fixing, though:

    diff --git a/libguile/goops.c b/libguile/goops.c
    index c2eb88f..c33bf0c 100644
    --- a/libguile/goops.c
    +++ b/libguile/goops.c
    @@ -193,6 +193,7 @@ static SCM scm_make_extended_class_from_symbol (SCM type_name_sym,
     						int applicablep);
     
     
    +

Please remove this empty-line insertion.

    @@ -437,16 +438,88 @@ remove_duplicate_slots (SCM l, SCM res, SCM slots_already_seen)
       return remove_duplicate_slots (SCM_CDR (l), res, slots_already_seen);
     }
     
    +static int has_class_super(SCM cplp)

"static int" on its own line, and a space before the paren, please.

    +{
    +  /* check to see if it is derived from <class> */
    +  for( ; scm_is_pair(cplp) ; cplp = SCM_CDR(cplp))

No spaces before the semicolons, and spaces before the parens.

    +    {
    +      if(SCM_CAR(cplp) == scm_class_class)
    +	{
    +	  return 1;
    +	}    
    +    }
    +  return 0;
    +}

No tabs, only spaces please.

    +
    +
    +static void check_cpl(SCM slots, SCM bslots)

Same, and no need for two blank lines.

    +{
    +  for(;scm_is_pair(bslots) ; bslots = SCM_CDR(bslots))
    +    {
    +      SCM pt;
    +      SCM b = SCM_CAAR(bslots);
    +      
    +      for(pt = slots  ;scm_is_pair(pt) ; pt = SCM_CDR(pt))
    +	{
    +	  if(SCM_CAAR(pt) == b)
    +	    scm_misc_error ("init-object", "a predefined <class> inherrited field cannot be redefined", SCM_EOL);
    +	  
    +	}
    +    }
    +}

Here we need spaces *after* each paren, and try to wrap lines to 72
chars, with a maximum of 80.  It is "inherited".

There are more style errors of this sort.  Can you post another patch
with these errors fixed?

Thanks,

Andy
-- 
http://wingolog.org/