GNOME Bugzilla – Bug 712837
gvariant: Document the need to cast varargs when constructing GVariants
Last modified: 2014-02-05 09:41:33 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.
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.
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...
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.
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.
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.
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.
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