Little Bug in Autobahn Android

#1

Hi Tobias, and everybody,

I’ve found a little bug in the Android library, and I say little because is not a plausible case, and also is very easy to fix, but I’m worried if such fix could introduce worse bugs, let me explain:
When making a rpc call, the wamp connection creates a call object, and then it fowards it to the writer and also puts it in the calls hashmap. When the response comes, its ID is matched with the calls hashmap. So, what’s the problem?: with an application in the emulator (and the server in localhost), it is “very” easy to produce (by stressing it actually) a race condition, where the call is written and the response comes so quickly that its call ID is not yet recorded in the calls hashmap, so it simply discards the result of the call and we never get a return value, or even an error, making it very hard to detect.
Code in the lines 288 and 289 of WampConnection looks like these:
mWriter.forward(call);
mCalls.put(call.mCallId, resultMeta);
If we simply invert the order of these lines:
mCalls.put(call.mCallId, resultMeta);
mWriter.forward(call);
We solved the bug described previously. Not without a cost however, we may then add objects to the hashmap, that can result in errors in the WampWriter, so we would have a bit of a memory leak in these cases. We may then use timeouts to wipe the calls that went wrong, or … simply ignore them (since they will be wiped anyway in the next connection reboot, since the hashmap belongs to the conection).

This is obviously not a big concern, but, what do you think? what should we do?

That’s all, regards,
Alejandro

0 Likes

#2

Hi Alejandro,

Hi Tobias, and everybody,

I've found a little bug in the Android library, and I say little because
is not a plausible case, and also is very easy to fix, but I'm worried
if such fix could introduce worse bugs, let me explain:
When making a rpc call, the wamp connection creates a call object, and
then it fowards it to the writer and also puts it in the calls hashmap.
When the response comes, its ID is matched with the calls hashmap. So,
what's the problem?: with an application in the emulator (and the server
in localhost), it is "very" easy to produce (by stressing it actually) a
race condition, where the call is written and the response comes so
quickly that its call ID is not yet recorded in the calls hashmap, so it

ah. ok. bad ..

simply discards the result of the call and we never get a return value,

you should at least see a log message when the call result/error cannot be matched to a call

https://github.com/tavendo/AutobahnAndroid/blob/master/Autobahn/src/de/tavendo/autobahn/WampReader.java#L140

or even an error, making it very hard to detect.
Code in the lines 288 and 289 of WampConnection looks like these:
       mWriter.forward(call);
       mCalls.put(call.mCallId, resultMeta);
If we simply invert the order of these lines:
       mCalls.put(call.mCallId, resultMeta);
       mWriter.forward(call);

looks fine for me. could you send a pull request on GitHub .. I merge.

We solved the bug described previously. Not without a cost however, we
may then add objects to the hashmap, that can result in errors in the
WampWriter, so we would have a bit of a memory leak in these cases. We
may then use timeouts to wipe the calls that went wrong, or ... simply
ignore them (since they will be wiped anyway in the next connection
reboot, since the hashmap belongs to the conection).

yes, I'd argue that is good enough. since when something goes wrong with the WampWriter, it's very likely that the cause is a loss of the underlying WebSocket connection, which will result in a new connection upon reconnect anyway ..

This is obviously not a big concern, but, what do you think? what should
we do?

That's all, regards,
Alejandro

Thanks for tracking this down (and sorry for the hassles;).

Tobias

···

Am 12.03.2013 21:42, schrieb Alejandro Hernandez:

--
You received this message because you are subscribed to the Google
Groups "Autobahn" group.
To unsubscribe from this group and stop receiving emails from it, send
an email to autobahnws+...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

0 Likes

#3

Hi Alejandro,

Hi Tobias, and everybody,

I’ve found a little bug in the Android library, and I say little because

is not a plausible case, and also is very easy to fix, but I’m worried

if such fix could introduce worse bugs, let me explain:

When making a rpc call, the wamp connection creates a call object, and

then it fowards it to the writer and also puts it in the calls hashmap.

When the response comes, its ID is matched with the calls hashmap. So,

what’s the problem?: with an application in the emulator (and the server

in localhost), it is “very” easy to produce (by stressing it actually) a

race condition, where the call is written and the response comes so

quickly that its call ID is not yet recorded in the calls hashmap, so it

ah. ok. bad …

simply discards the result of the call and we never get a return value,

you should at least see a log message when the call result/error cannot
be matched to a call

https://github.com/tavendo/AutobahnAndroid/blob/master/Autobahn/src/de/tavendo/autobahn/WampReader.java#L140

You’re right, you get a warning in the catlog, but I meant that the application using the library doesn’t get anything to handle with, is silent from its point of view. I wonder if perhaps raising an exception instead of a simple warning would be better, but of course, that would introduce its own problems, for instance, it would be pretty easy to attack an application just by forking a fake call response and making it crash. So I think its ok by now.

or even an error, making it very hard to detect.

Code in the lines 288 and 289 of WampConnection looks like these:

   mWriter.forward(call);
   mCalls.put(call.mCallId, resultMeta);

If we simply invert the order of these lines:

   mCalls.put(call.mCallId, resultMeta);
   mWriter.forward(call);

looks fine for me. could you send a pull request on GitHub … I merge.

Done.

We solved the bug described previously. Not without a cost however, we

may then add objects to the hashmap, that can result in errors in the

WampWriter, so we would have a bit of a memory leak in these cases. We

may then use timeouts to wipe the calls that went wrong, or … simply

ignore them (since they will be wiped anyway in the next connection

reboot, since the hashmap belongs to the conection).

yes, I’d argue that is good enough. since when something goes wrong with
the WampWriter, it’s very likely that the cause is a loss of the
underlying WebSocket connection, which will result in a new connection
upon reconnect anyway …

I would like to note, that in the subscribe method there is a similar condition, where the writer could be faster than the hashmap, so you will probably loose a first incoming message. Although, is not really a problem, since publish/subscribe is supposed to be fully asynchronous, so such cases (of lost messages) should be handled by the application logic anyway.

Also, I think there is a little bug in the unsubscribe method, since it sends the unsubscribe message to the writer, but it never removes the subscription from the mSubs hashmap, It will be cleared in the next reconnection anyway, but for a small time span it will still receive messages from such subscription, and that might be a vulnerability.

I never had actual practical problems with these methods, but I’m letting you know just in case. I could fix those to if you want.

This is obviously not a big concern, but, what do you think? what should

we do?

That’s all, regards,

Alejandro

Thanks for tracking this down (and sorry for the hassles;).

No problem, thanks for such a great library, I’m using it in a couple of projects now, with very good results.

Tobias

You received this message because you are subscribed to the Google

Groups “Autobahn” group.

To unsubscribe from this group and stop receiving emails from it, send

an email to autobahnws+...@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

Regards,
Alejandro.

···

On Saturday, March 16, 2013 5:05:11 AM UTC-4:30, Tobias Oberstein wrote:

Am 12.03.2013 21:42, schrieb Alejandro Hernandez:

0 Likes

#4

    looks fine for me. could you send a pull request on GitHub .. I merge.

Done.

Just merged. Thanks!

I would like to note, that in the subscribe method there is a similar
condition, where the writer could be faster than the hashmap, so you
will probably loose a first incoming message. Although, is not really a
problem, since publish/subscribe is supposed to be fully asynchronous,
so such cases (of lost messages) should be handled by the application
logic anyway.

Also, I think there is a little bug in the unsubscribe method, since it
sends the unsubscribe message to the writer, but it never removes the
subscription from the mSubs hashmap, It will be cleared in the next
reconnection anyway, but for a small time span it will still receive
messages from such subscription, and that might be a vulnerability.

I never had actual practical problems with these methods, but I'm
letting you know just in case. I could fix those to if you want.

Would be just consequent to do it likewise for above. So yes please, I'll merge ..

Thanks for contributing!

- Tobias

0 Likes