#1339 - ODBC crashes vm

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

#1339 - ODBC crashes vm

Schwab,Wilhelm K
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
Reply | Threaded
Open this post in threaded view
|

Re: #1339 - ODBC crashes vm

Mariano Martinez Peck


On Mon, Oct 19, 2009 at 2:34 PM, Schwab,Wilhelm K <[hidden email]> wrote:
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: I could reproduce it in both OS. I saw there is some problem when the debugger stack is stored to disk. Could you please evaluate

Preferences disable: #logDebuggerStackToFile

And try again with valid DSN, user and password.

Any difference?  In my case the segmentation fault is in ODBCConnection >> sqlAllocConnect:


cheers

mariano
 
Bill


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


_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: #1339 - ODBC crashes vm

Henrik Sperre Johansen
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
Reply | Threaded
Open this post in threaded view
|

Re: #1339 - ODBC crashes vm

Mariano Martinez Peck


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


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


_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: #1339 - ODBC crashes vm

Henrik Sperre Johansen

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
Reply | Threaded
Open this post in threaded view
|

Re: #1339 - ODBC crashes vm

Mariano Martinez Peck


2009/10/22 Henrik Johansen <[hidden email]>

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 

I think there there was a misunderstood. The one who has the problem was Schwab,Wilhelm
 


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.


I dont understand. In SqueakDBX (not ODBC) I have never had to send the message defineFields. It is sent by the class side initialize. So, I don't forget it. It is automatically. When you download SqueakDBX from Monticello, that's is done.
 
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.



You forgot to upload the changeset ;)


 
Cheers,
Henry




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


_______________________________________________
Pharo-project mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Reply | Threaded
Open this post in threaded view
|

Re: #1339 - ODBC crashes vm

Schwab,Wilhelm K
Correction, I reported the problem, but anyone trying to use ODBC on Pharo will have a bad day until we fix something.  Whether or not a bug lives in Pharo is of less interest to potential users (and to us!) than is having working code.
 
Bill

 


From: [hidden email] [mailto:[hidden email]] On Behalf Of Mariano Martinez Peck
Sent: Friday, October 23, 2009 8:21 AM
To: [hidden email]
Subject: Re: [Pharo-project] #1339 - ODBC crashes vm



2009/10/22 Henrik Johansen <[hidden email]>

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 

I think there there was a misunderstood. The one who has the problem was Schwab,Wilhelm
 


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.


I dont understand. In SqueakDBX (not ODBC) I have never had to send the message defineFields. It is sent by the class side initialize. So, I don't forget it. It is automatically. When you download SqueakDBX from Monticello, that's is done.
 
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.



You forgot to upload the changeset ;)


 
Cheers,
Henry




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


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