Created attachment 163344 [details] [review]
Don't deref C Arrays

So this isn't a complete fix.  While it fixes some offset bugs, it doesn't get them all. The issue is with the GtkStyle object.  Here is sample code snippet from PyGI which prints all the values:

print style.fg
print style.bg
print style.light
print style.dark
print style.mid
print style.text
print style.base
print style.text_aa

print style.black
print style.white
print style.font_desc

print style.xthickness
print style.ythickness

# Above seems to return correct values

# not sure if the next three are correct but bg_gc[Gtk.StateType.NORMAL]
# does not have the same color values as style.bg[Gtk.StateType.NORMAL]
print style.fg_gc
print style.bg_gc
print style.light_gc
            
# segfault here
print style.dark_gc
print style.mid_gc
print style.text_gc
print style.base_gc
print style.text_aa_gc
print style.black_gc
print style.white_gc

print style.bg_pixmap
print gv.background

Output:

(<Gdk.Color(red=0, green=0, blue=0)>, <Gdk.Color(red=0, green=0, blue=0)>, <Gdk.Color(red=0, green=0, blue=0)>, <Gdk.Color(red=65535, green=65535, blue=65535)>, <Gdk.Color(red=30000, green=30000, blue=30000)>)
(<Gdk.Color(red=0, green=0, blue=65535)>, <Gdk.Color(red=50372, green=49858, blue=48573)>, <Gdk.Color(red=61166, green=60395, blue=59367)>, <Gdk.Color(red=19275, green=26985, blue=33667)>, <Gdk.Color(red=56540, green=56026, blue=54741)>)
(<Gdk.Color(red=19660, green=19660, blue=65534)>, <Gdk.Color(red=64403, green=64352, blue=64225)>, <Gdk.Color(red=65535, green=65535, blue=65535)>, <Gdk.Color(red=23413, green=35197, blue=45411)>, <Gdk.Color(red=65534, green=65534, blue=65535)>)
(<Gdk.Color(red=6881, green=6881, blue=38993)>, <Gdk.Color(red=35842, green=35149, blue=33419)>, <Gdk.Color(red=44976, green=42585, blue=39396)>, <Gdk.Color(red=15003, green=18781, blue=22055)>, <Gdk.Color(red=40640, green=39673, blue=37256)>)
(<Gdk.Color(red=13270, green=13270, blue=52263)>, <Gdk.Color(red=50122, green=49750, blue=48822)>, <Gdk.Color(red=55255, green=54060, blue=52465)>, <Gdk.Color(red=19208, green=26989, blue=33733)>, <Gdk.Color(red=53087, green=52603, blue=51395)>)
(<Gdk.Color(red=0, green=0, blue=0)>, <Gdk.Color(red=65535, green=65535, blue=65535)>, <Gdk.Color(red=0, green=0, blue=0)>, <Gdk.Color(red=65535, green=65535, blue=65535)>, <Gdk.Color(red=30000, green=30000, blue=30000)>)
(<Gdk.Color(red=65535, green=65535, blue=65535)>, <Gdk.Color(red=40092, green=39578, blue=38036)>, <Gdk.Color(red=65535, green=65535, blue=65535)>, <Gdk.Color(red=19275, green=26985, blue=33667)>, <Gdk.Color(red=61166, green=60395, blue=59367)>)
(<Gdk.Color(red=32767, green=32767, blue=32767)>, <Gdk.Color(red=52813, green=52556, blue=51785)>, <Gdk.Color(red=32767, green=32767, blue=32767)>, <Gdk.Color(red=42405, green=46260, blue=49601)>, <Gdk.Color(red=45583, green=45197, blue=44683)>)
<Gdk.Color(red=0, green=0, blue=0)>
<Gdk.Color(red=65535, green=65535, blue=65535)>
<PangoFontDescription at 0x11a6ac0>
2
2
(<gtk.gdk.GCX11 object at 0x7f562af80a00 (GdkGCX11 at 0x1199290)>, <gtk.gdk.GCX11 object at 0x7f562af80a00 (GdkGCX11 at 0x1199290)>, <gtk.gdk.GCX11 object at 0x7f562af80a00 (GdkGCX11 at 0x1199290)>, <gtk.gdk.GCX11 object at 0x7f562af80a50 (GdkGCX11 at 0x11993d0)>, <gtk.gdk.GCX11 object at 0x7f562af80aa0 (GdkGCX11 at 0x11f96f0)>)
(<gtk.gdk.GCX11 object at 0x7f562af80a00 (GdkGCX11 at 0x11993d0)>, <gtk.gdk.GCX11 object at 0x7f562af80a50 (GdkGCX11 at 0x11f9150)>, <gtk.gdk.GCX11 object at 0x7f562af80a00 (GdkGCX11 at 0x11993d0)>, <gtk.gdk.GCX11 object at 0x7f562af80aa0 (GdkGCX11 at 0x11f9470)>, <gtk.gdk.GCX11 object at 0x7f562af80af0 (GdkGCX11 at 0x11f9290)>)
(None, None, None, None, None)
Segmentation fault (core dumped)


I will look at it more in depth tomorrow

Further update for those who are keeping score:

It seems that we need to handle Obj [], Obj *[] and perhaps Obj ** differently so the patch is most certainly not complete.  It has been a long time since I have dealt with this level of pointer manipulation but we might need more annotations or to set is_pointer to False.  I'll try a couple of things.  If anyone else has any ideas please feel free to comment.

Created attachment 163410 [details] [review]
Don't deref C Arrays

* When dealing with C arrays we just want the pointer to the head of the
  array.

Review of attachment 163344 [details] [review]:

This patch would make it impossible to represent a pointer to an array in a structure, that is, you couldn't have:

  int **values;

I think it's impossible to say what is correct without coming to a firm understanding of the mysterious "is_pointer" flag.

Review of attachment 163410 [details] [review]:

Oh, should have reviewed this one instead. But I think it's clearly wrong to say that the zero-terminated flag flips us between

 int values[<length>]

and:

 int *values;

zero-terminated is *not* about the degree of indirection.

Further analysis gives us another bug.  The patch sorta kinda fixes the deref
bug but the checks for when we should be derefing the pointer and just
returning the raw pointer aren't completely right.

The second bug is an offset calculation bug.  It seems for fields of the type
such as GdkGC*[] use GdkGC and not GdkGC* to calculate the next offset.  In the
above segfault, field fg_gc is being read correctly but the next field is
calculated wrong because of the GdkGC* memebers inside of the fg_gc array.

(In reply to comment #6)
> Review of attachment 163410 [details] [review]:
> 
> Oh, should have reviewed this one instead. But I think it's clearly wrong to
> say that the zero-terminated flag flips us between
> 
>  int values[<length>]
> 
> and:
> 
>  int *values;
> 
> zero-terminated is *not* about the degree of indirection.

Right, I figured that out but right now it is the best option I had to make the tests pass and move forward with analysing the bug.  I think allowing us to override is_pointer and making !is_pointer be an alias for is_fixed for arrays would work.

The other option is to just make accessor functions and have GI block out direct access to ambiguous arrays

I found the offset error by manually editing the Gir file and passing it through the compiler:

     <field name="fg_gc">
        <array zero-terminated="0" c:type="GdkGC*" fixed-size="5">
          <type name="Gdk.GC" c:type="GdkGC*"/>
        </array>
      </field>
      <field name="bg_gc">
        <array zero-terminated="0" c:type="GdkGC*" fixed-size="5">
          <type name="Gdk.GC"/>
        </array>
      </field>

in this example fg_gc now calculates GdkGC correctly because I added c:type= to the type tag which then correctly picks up that the type is a pointer.  The c:type= in the array tag does not effect this. 

Is the GIR file being output wrong or is is_pointer reading the gir incorrectly?  From the annotations where you can set an array's element type it seems that the intention is for the array's c:type to be inherited by the element type.

I've tried to remove the is_pointer flag in the past; see bug 561099.

What remains to be done here is just adding all these array variants to the test suite and making sure the GIR can represent them.

The typelib is going to need more work, and bindings yet further work.

I'd like to defer on this bug for now until I can land the scanner rewrite from bug 622609, then we can unblock on further GIR/typelib API changes.

Fine to defer but this is a huge issue which causes weird bugs to pop up (I think a disappearing attr is related to bad memory access caused by this bug).  It also may be able to be and exploitable hole.

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]