After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 712837 - gvariant: Document the need to cast varargs when constructing GVariants
gvariant: Document the need to cast varargs when constructing GVariants
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gvariant
unspecified
Other All
: Normal normal
: ---
Assigned To: Allison Karlitskaya (desrt)
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-11-21 17:47 UTC by Philip Withnall
Modified: 2014-02-05 09:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gvariant: Document the need to cast varargs when constructing GVariants (6.29 KB, patch)
2013-11-21 17:47 UTC, Philip Withnall
needs-work Details | Review
gvariant: Document the need to cast varargs when constructing GVariants (5.91 KB, patch)
2013-11-25 10:58 UTC, Philip Withnall
none Details | Review
gvariant: Document the need to cast varargs when constructing GVariants (6.02 KB, patch)
2014-02-03 23:10 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2013-11-21 17:47:38 UTC
Here’s a documentation patch to warn about the problems associated with passing variables into GVariant construction functions whose types don’t exactly match those given in the GVariant type string. In normal C, this would be fine due to implicit casting inserted by the compiler; but since the compiler doesn’t have access to type information from the GVariant type string, we have to do explicit casting in those situations.
Comment 1 Philip Withnall 2013-11-21 17:47:40 UTC
Created attachment 260477 [details] [review]
gvariant: Document the need to cast varargs when constructing GVariants

When passing values to a GVariant construction function like
g_variant_new() or g_variant_builder_add(), the type information for the
varargs is not available to the compiler. This means that it cannot
perform implicit type casts to ensure the variables are of the correct
width on the call stack. This can result in stack corruption if, for
example, a boolean variable (which has the same width as the word size)
is passed in for a guint64 (type string ‘t’) argument. On 32-bit
machines, 4 bytes will be pushed onto the stack, but g_variant_new()
will pop 8 bytes off. This is not limited to 32-bit machines; other
situations can occur on 64-bit machines.

The only way to avoid this is to insert explicit casts for parameters to
g_variant_new() and friends where the type of the variable or constant
being passed does not exactly match the corresponding argument in the
GVariant type string.

Add big scary documentation warning about this, since it can result in
some really subtle and hard-to-track-down bugs.
Comment 2 Allison Karlitskaya (desrt) 2013-11-21 18:13:42 UTC
Review of attachment 260477 [details] [review]:

We already have (more factually accurate) documentation for this here: https://developer.gnome.org/glib/stable/gvariant-format-strings.html

Namely:

"""

 Note that in C, small integer types in variable argument lists are promoted up to int or unsigned int as appropriate, and read back accordingly. int is 32 bits on every platform on which GLib is currently suported. This means that you can use C expressions of type int with g_variant_new() and format characters 'b', 'y', 'n', 'q', 'i', 'u' and 'h'. Specifically, you can use integer literals with these characters.

 When using the 'x' and 't' characters, you must ensure that the value that you provide is 64 bit. This means that you should use a cast or make use of the G_GINT64_CONSTANT or G_GUINT64_CONSTANT macros. 

"""

I don't mind adding additional notices elsewhere, but we should probably ensure that they are precise and correct.  For example, casting a bool to an int64 is a weird example and the talk about "stack corruption" is simply untrue.  In general, I'd also prefer if users of all vararg funcs were directed back to the documentation for GVariant varargs instead of for g_variant_new().  I also don't think it's really necessary to have pointers from the _va variants...
Comment 3 Philip Withnall 2013-11-25 10:58:46 UTC
Created attachment 261418 [details] [review]
gvariant: Document the need to cast varargs when constructing GVariants

Slightly expand on the documentation about casting varargs when
constructing GVariants, and link to it from all the functions where it’s
a necessary consideration.

Add an example of passing flags to a ‘t’ type variable (guint64).
Assuming the flags enum does not have many members, the flag variable
will be 32 bits wide, and needs an explicit cast to be passed into
g_variant_new() as a 64-bit value.
Comment 4 Allison Karlitskaya (desrt) 2014-01-21 21:48:36 UTC
Review of attachment 261418 [details] [review]:

::: glib/gvariant.c
@@ +4707,3 @@
+ * <programlisting>
+ * MyFlags some_flags = FLAG_ONE | FLAG_TWO;
+ * GStrv *some_strings = { "a", "b", "c", NULL };

This assignment is not valid -- string literals have type 'const char *', so 'const gchar **' is probably best here.

@@ +4710,3 @@
+ * GVariant *new_variant;
+ *
+ * new_variant = g_variant_new ("tas",

this is not a valid format string.  did you mean to construct a tuple?

also: 'as' is not the correct format string for a strv.
Comment 5 Philip Withnall 2014-02-03 23:10:20 UTC
Created attachment 268016 [details] [review]
gvariant: Document the need to cast varargs when constructing GVariants

Slightly expand on the documentation about casting varargs when
constructing GVariants, and link to it from all the functions where it’s
a necessary consideration.

Add an example of passing flags to a ‘t’ type variable (guint64).
Assuming the flags enum does not have many members, the flag variable
will be 32 bits wide, and needs an explicit cast to be passed into
g_variant_new() as a 64-bit value.
Comment 6 Allison Karlitskaya (desrt) 2014-02-04 09:31:58 UTC
Review of attachment 268016 [details] [review]:

Looks good with two minor changes.

Thanks for putting up with all the back-and-forth :)

::: glib/gvariant.c
@@ +86,3 @@
  * For convenience to C programmers, #GVariant features powerful
  * varargs-based value construction and destruction.  This feature is
+ * designed to be embedded in other libraries. Note that values must be

Please drop this modification.

@@ +4662,3 @@
  *
+ * Note that the arguments must be of the correct width for their types
+ * specified in @format_string, and this can be achieved by casting them. See

Change this to read:

"...specified in @format_string.  This can be achieved..."

ie: don't use "and" here.
Comment 7 Philip Withnall 2014-02-05 09:41:28 UTC
Pushed to master with those modifications. Thanks for the attentive reviews.

Attachment 268016 [details] pushed as 2b8edf2 - gvariant: Document the need to cast varargs when constructing GVariants