Bug 45236 - Gabble plugin crashes when advertising/receiving a service description with exactly one name localization
Summary: Gabble plugin crashes when advertising/receiving a service description with e...
Status: RESOLVED FIXED
Alias: None
Product: Ytstenut
Classification: Unclassified
Component: plugins (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact:
URL: http://cgit.collabora.com/git/user/og...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2012-01-25 10:23 UTC by Olli Salli
Modified: 2012-08-06 18:50 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

Attachments
Explicitly specify the form type of every Yts form field (1.41 KB, patch)
2012-08-06 12:34 UTC, Simon McVittie 		Details | Splinter Review
*/status.c: don't crash if form fields have unexpected types (4.29 KB, patch)
2012-08-06 12:34 UTC, Simon McVittie 		Details | Splinter Review
View All

Description Olli Salli 2012-01-25 10:23:11 UTC 
Hit this quite weird bug when experimenting with changes to some of the tp-ytstenut tests similar to Danilo's patch in bug 45218. I noticed that when I advertise exactly one name for my service with tp_yts_client_add_name, gabble crashes.

It did, because it receives the data form it just sent as per XEP-0115, and parses it (though then drops it on the floor because it's the local user). In parsing, Wocky uses a different GType to represent values of dataform fields with a single entry and multiple ones. The code expected it to always be the multiple values one.

Remote gabbles receiving the same service description would also crash.

The URL has a branch which makes this part more resilient and behave correctly with an arbitrary number of names.
Comment 1 Simon McVittie 2012-08-06 12:34:21 UTC 
Created attachment 65171 [details] [review]
Explicitly specify the form type of every Yts form field

Otherwise, the client side will have to guess it: text-multi if there
is more than one of something, otherwise text-single.
Comment 2 Simon McVittie 2012-08-06 12:34:42 UTC 
Created attachment 65172 [details] [review]
*/status.c: don't crash if form fields have unexpected types

FORM_TYPE should be hidden, type should be text-single, and name and caps
should be text-multi.
 
This is significant because if the types of name and caps are not
explicitly specified in the message, Wocky will guess. If there is only
one value, it will guess text-single, and the GValue will contain
G_TYPE_STRING, not G_TYPE_STRV, causing a crash when we call
g_value_get_boxed.
Comment 3 Simon McVittie 2012-08-06 12:40:39 UTC 
(In reply to comment #0)
> I noticed that when I
> advertise exactly one name for my service with tp_yts_client_add_name, gabble
> crashes.

Also Salut, for which I did the attached patches. I haven't tested them with Gabble but the code is the same.

> In
> parsing, Wocky uses a different GType to represent values of dataform fields
> with a single entry and multiple ones.

The GType is fixed per field type; the problem was that the implementation assumed the field type would always be text-multi. That's a "reader" bug.

In addition, I think sending multi-valued fields without setting their type is a "writer" bug, because the default (per XEP-0004) is text-single.
Comment 4 Olli Salli 2012-08-06 18:38:35 UTC 
Comment on attachment 65171 [details] [review]
Explicitly specify the form type of every Yts form field

Review of attachment 65171 [details] [review]:
-----------------------------------------------------------------

Yeah, I suppose it's better to always use text-multi.
Comment 5 Olli Salli 2012-08-06 18:43:37 UTC 
Comment on attachment 65172 [details] [review]
*/status.c: don't crash if form fields have unexpected types

Review of attachment 65172 [details] [review]:
-----------------------------------------------------------------

Well, this will cause the plugins to ignore, rather than gracefully parse, the text-single name and caps fields the old code could produce. But that's OK, as long as we don't crash.
Comment 6 Simon McVittie 2012-08-06 18:50:33 UTC 
Merged, thanks.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.