#1339 - ODBC crashes vm

Hello all,

I decided to try the simplest of all possible tests to reproduce my ODBC crash: start with a fresh beta image, load FFI and ODBC, and then try to connect to a non-existent data source.  It indeed crashes the Linux VM, and the Windows VM hangs badly.

Sorry about this next part: I marked it for 1.0.

Bill


_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

This one is caused by an initialization bug of some sort, the
byteSizes of ODBC-structs are incorrect.

In short, the solution is to do:
ExternalStructure allSubclassesDo: [:each | each compileFields]
after you load a package which defines ExternalStructure subclasses
(like ODBC does).

For some reason, in an image after loading:

ExternalStrucure >> byteSize
       "Return the size in bytes of this structure."
       ^self compiledSpec first bitAnd: FFIStructSizeMask

SQLHENV compiledSpec first: 65536
FFIStructSizeMask: 65535...

So basically, when you allocate the struct a handle of a ByteArray of
size byteSize,  you'll sooner or later you access unallocated memory
and crash.
Recomputing all the compiledSpecs with the doit above leads to the
correct sizes (4 in the case of SQLHENV) being stored in the
compiledSpecs.

Either way, not raising an error when the size for a struct when
and'ed with the mask,  but instead causing a silent crash later on
just seems bad bad bad to me.

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

On Oct 20, 2009, at 4:16 37PM, Mariano Martinez Peck wrote:

On Tue, Oct 20, 2009 at 8:06 AM, Henrik Johansen <[hidden email]> wrote:

This one is caused by an initialization bug of some sort, the
byteSizes of ODBC-structs are incorrect.

In short, the solution is to do:
ExternalStructure allSubclassesDo: [:each | each compileFields]
after you load a package which defines ExternalStructure subclasses
(like ODBC does).

Hi Henrik! Nice to see you arround ;)

In SqueakDBX I use FFI, I have subclasses of ExternalStructure  and I don't remember having to do that. The only thing I do in those subclasses is to implement a class side method initialize like this:

initialize
    super initialize.
    self defineFields
 

And then, if you see defineFields you will se that at the end, it does a compileField...
You can see defineFields in FFI documentation: http://wiki.squeak.org/squeak/2426

So, in my opinion a better solution may be this. What do you think ?

Cheers

Mariano

Errr, so in conclusion, you didn't do exactly what I did, but something else which results in doing exactly the same, but in addition define accessors for the fields.

So you did have to do that, even though you don't remember it. :P 

Here's the reasons why I feel the solution you found, by itself, does not constitute a good solution:

- If you DON'T remember to do it, the image of people loading your project might crash silently.

- Neither FFI itself nor ODBC has remembered to do so (it's done in the loadFFI script in ScriptLoader)

- Debugging it if you haven't read some arbitrary wiki-page was a real pain (partly because it doesn't crash as soon as the error is made)

- Even you, who had actually read the wikipage, didn't make the association to this issue.

A couple of alternative approaches:

a) - Returning an error if a basicSize request would return 0 informing that fields have not been properly initialized (Or silently doing a recompile)

b) Wait for MC1.6's atomic loader so you can be sure the fields method is compiled when a new external structure is installed :)

c) Don't store into compiledSpec if fields is empty, on the presumption it will not be used until fields has been defined. 

(This won't actually work, as loading from MC will only execute class initialization only if class initialize method is defined in package being loaded (store works the same), but:

d) - Do a compileFields: call in ExternalStructure>>initialize, and instead create a createAccessors method which can be overridden if necessary. )

a) and c) will both suffer from the issue that if you load/merge a newer version with additional fields defined, you get the same flaky behaviour as is experienced now.

b) is probably some time off in the future, depending on when someone gets the time to implement trait support.

a), but without the silent recompile and a helpful error message might be the best short-term option, since it'd remind authors to add class initialize-methods, and tell a user what to do if they've forgotten to do so instead of crashing the image. 

I've attached a changeset implementing c) to the issue.

Cheers,

Henry

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

On Oct 20, 2009, at 4:16 37PM, Mariano Martinez Peck wrote:

On Tue, Oct 20, 2009 at 8:06 AM, Henrik Johansen <[hidden email]> wrote:

This one is caused by an initialization bug of some sort, the
byteSizes of ODBC-structs are incorrect.

In short, the solution is to do:
ExternalStructure allSubclassesDo: [:each | each compileFields]
after you load a package which defines ExternalStructure subclasses
(like ODBC does).

Hi Henrik! Nice to see you arround ;)

In SqueakDBX I use FFI, I have subclasses of ExternalStructure  and I don't remember having to do that. The only thing I do in those subclasses is to implement a class side method initialize like this:

initialize
    super initialize.
    self defineFields
 

And then, if you see defineFields you will se that at the end, it does a compileField...
You can see defineFields in FFI documentation: http://wiki.squeak.org/squeak/2426

So, in my opinion a better solution may be this. What do you think ?

Cheers

Mariano

Errr, so in conclusion, you didn't do exactly what I did, but something else which results in doing exactly the same, but in addition define accessors for the fields.

So you did have to do that, even though you don't remember it. :P 

Here's the reasons why I feel the solution you found, by itself, does not constitute a good solution:

- If you DON'T remember to do it, the image of people loading your project might crash silently.

- Neither FFI itself nor ODBC has remembered to do so (it's done in the loadFFI script in ScriptLoader)

- Debugging it if you haven't read some arbitrary wiki-page was a real pain (partly because it doesn't crash as soon as the error is made)

- Even you, who had actually read the wikipage, didn't make the association to this issue.

A couple of alternative approaches:

a) - Returning an error if a basicSize request would return 0 informing that fields have not been properly initialized (Or silently doing a recompile)

b) Wait for MC1.6's atomic loader so you can be sure the fields method is compiled when a new external structure is installed :)

c) Don't store into compiledSpec if fields is empty, on the presumption it will not be used until fields has been defined. 

(This won't actually work, as loading from MC will only execute class initialization only if class initialize method is defined in package being loaded (store works the same), but:

d) - Do a compileFields: call in ExternalStructure>>initialize, and instead create a createAccessors method which can be overridden if necessary. )

a) and c) will both suffer from the issue that if you load/merge a newer version with additional fields defined, you get the same flaky behaviour as is experienced now.

b) is probably some time off in the future, depending on when someone gets the time to implement trait support.

a), but without the silent recompile and a helpful error message might be the best short-term option, since it'd remind authors to add class initialize-methods, and tell a user what to do if they've forgotten to do so instead of crashing the image. 

I've attached a changeset implementing c) to the issue.

Cheers,

Henry

_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project