SIGSEGV in Java bindings

[simoncn]

There's a problem in the Java bindings that can cause a SIGSEGV crash when a control point sends invalid XML to a server.

In DvInvocation.c, the DvInvocationReadString and DvInvocationReadBinary native methods are missing an error check that can cause them to use pointer and length values that haven't been set after a call to DvInvocationReadStringAsBuffer or DvInvocationReadBinary in the C bindings.

A patch is attached. I have tested this to confirm that it solves the problem.

[simonc]

Thanks! I've applied the patch for DvInvocation.c. All being well, it'll be on github this evening.

I noticed that a similar problem exists in other source files too. We'll try to fix these in the next couple of weeks.

I haven't applied the patch for Invocation.c yet. I read them as "swap use of AttachCurrentThread for AttachCurrentThreadAsDaemon"; these changes were already present for me. Have I misunderstood what should be added/removed? If not, is it possible you're working from an older checkout?

[simoncn]

(24-09-2014 09:38 AM)simonc Wrote:  Thanks! I've applied the patch for DvInvocation.c. All being well, it'll be on github this evening.

I noticed that a similar problem exists in other source files too. We'll try to fix these in the next couple of weeks.

I haven't applied the patch for Invocation.c yet. I read them as "swap use of AttachCurrentThread for AttachCurrentThreadAsDaemon"; these changes were already present for me. Have I misunderstood what should be added/removed? If not, is it possible you're working from an older checkout?

Thanks very much! The patch for Invocation.c shouldn't have been included. It is old and was applied previously. Apologies for the confusion.

If you'd like to point me in the direction of the other source files with the same problem, I'd be happy to have a go at producing fixes for these as well.

[simonc]

(24-09-2014 02:50 PM)simoncn Wrote:  If you'd like to point me in the direction of the other source files with the same problem, I'd be happy to have a go at producing fixes for these as well.

Thanks, I'd really appreciate that! I've double checked the code and the list of suspect calls is shorter than I'd originally thought:

• ServicePropertyValueString (PropertyString.c)
• ServicePropertyGetValueBinary (PropertyBinary.c)
  

both appear to read back copies of strings so should cope with receiving an error.

[simoncn]

(24-09-2014 03:21 PM)simonc Wrote:  

(24-09-2014 02:50 PM)simoncn Wrote:  If you'd like to point me in the direction of the other source files with the same problem, I'd be happy to have a go at producing fixes for these as well.

Thanks, I'd really appreciate that! I've double checked the code and the list of suspect calls is shorter than I'd originally thought:

• ServicePropertyValueString (PropertyString.c)
• ServicePropertyGetValueBinary (PropertyBinary.c)
  

both appear to read back copies of strings so should cope with receiving an error.

I see this problem in PropertyBinary.c. In the ServicePropertyGetValueBinary native method, the return value from the C bindings call

ServicePropertyGetValueBinary(property, &data, &len);

isn't checked. The fix for this is straightforward.

In PropertyString.c, things are more complicated. The ServicePropertyValueString native method calls ServicePropertyValueString in the C bindings, which doesn't return an error code. The code for this method in the C bindings includes the comment

// FIXME - no handling of PropertyError

There is also a similar method ServicePropertyGetValueString in the C bindings which does catch PropertyError and return an error code, and I think the Java bindings should be using ServicePropertyGetValueString instead of ServicePropertyValueString. This fix should also include using the correct UTF-8 encodings when reading and writing property string values as was done a few months ago for DVInvocation.c.

There is a further complication that the Java versions of the ServicePropertyGetValueBinary and ServicePropertyValueString methods in PropertyBinary.java and PropertyString.java don't allow for receiving an error code or throwing an exception back to user code to report this error. In the C# bindings, this is handled by throwing a PropertyError if any of the ServicePropertyXXX getter calls returns -1. For consistency, the Java bindings should do the same.

I will produce a patch to fix these problems in the next few days.

[simoncn]

(24-09-2014 04:40 PM)simoncn Wrote:  I see this problem in PropertyBinary.c. In the ServicePropertyGetValueBinary native method, the return value from the C bindings call

ServicePropertyGetValueBinary(property, &data, &len);

isn't checked. The fix for this is straightforward.

In PropertyString.c, things are more complicated. The ServicePropertyValueString native method calls ServicePropertyValueString in the C bindings, which doesn't return an error code. The code for this method in the C bindings includes the comment

// FIXME - no handling of PropertyError

There is also a similar method ServicePropertyGetValueString in the C bindings which does catch PropertyError and return an error code, and I think the Java bindings should be using ServicePropertyGetValueString instead of ServicePropertyValueString. This fix should also include using the correct UTF-8 encodings when reading and writing property string values as was done a few months ago for DVInvocation.c.

There is a further complication that the Java versions of the ServicePropertyGetValueBinary and ServicePropertyValueString methods in PropertyBinary.java and PropertyString.java don't allow for receiving an error code or throwing an exception back to user code to report this error. In the C# bindings, this is handled by throwing a PropertyError if any of the ServicePropertyXXX getter calls returns -1. For consistency, the Java bindings should do the same.

I will produce a patch to fix these problems in the next few days.

The patch is attached. There is one new file:

OpenHome/Net/Bindings/Java/org/openhome/net/core/PropertyError.java

and eight changed files:

Common.mak
OpenHome/Net/Bindings/Java/PropertyBool.c
OpenHome/Net/Bindings/Java/PropertyInt.c
OpenHome/Net/Bindings/Java/PropertyBinary.c
OpenHome/Net/Bindings/Java/PropertyUint.c
OpenHome/Net/Bindings/Java/PropertyString.c
OpenHome/Net/Bindings/C/ServiceC.cpp
OpenHome/Net/Bindings/C/Service.h

The changes are as described above with one addition. To implement the UTF-8 change in the ServicePropertySetValueString Java native method, it was necessary to add a new function ServicePropertySetValueStringAsBuffer to ServiceC.cpp in the C bindings. This is similar to the function DvInvocationWriteStringAsBuffer that was added to DvProviderC.cpp for the previous set of UTF-8 changes.

I hvae tested these changes, including verifying that a Java PropertyError exception is thrown instead of the process being terminated because of an uncaught C++ PropertyError exception.

[simonc]

Thanks! I'll get these changes applied tomorrow.

[simoncn]

(08-10-2014 09:11 PM)simonc Wrote:  Thanks! I'll get these changes applied tomorrow.

Thanks for doing this. Unfortunately, there is a problem with how the changes were applied to PropertyBinary.c. In this listing, lines 80, 83 and 84 need to be deleted.

[simonc]

(09-10-2014 10:08 PM)simoncn Wrote:  Thanks for doing this. Unfortunately, there is a problem with how the changes were applied to PropertyBinary.c. In this listing, lines 80, 83 and 84 need to be deleted.

Thanks for catching that. Should be fixed now.

[simoncn]

(10-10-2014 08:39 AM)simonc Wrote:  Thanks for catching that. Should be fixed now.

I've run diffs on all the other files and I found a similar problem in PropertyBool.c. In this listing, line 78 should be deleted.