Created attachment 263673 [details] [review]
g_settings_get: check validity of format string

Allow only format strings that copy all values (i.e, don't contain '&'),
as the returned pointers might become invalid in some rare cases.

Since this is technically an API break, this patch only prints a
critical when a faulty format string is detected, but still fetches the
values.

Review of attachment 263673 [details] [review]:

I'd argue that this is technically not an API break because not copying doesn't work properly in all cases.

Looks good for now, thanks.  Let's see what it kicks up.

Attachment 263673 [details] pushed as 396d40a - g_settings_get: check validity of format string

Could you specify exactly under which circumstances e.g. a g_settings_get (s, "&s", &str) could fail to work properly?

That depends on the backend. g_settings_get() gets a GVariant from the backend, calls g_variant_get_va(), and unrefs the variant. There's no guarantee that the variant stays alive after that.

In practice this happens can happen in the dconf backend when the backing gvdb gets replaced. If I understood Ryan on irc earlier today correctly, this sequence should do it:

  g_settings_get (settings, "some-key", "&s", &str);

  g_settings_set_value (settings, "some-other-key", bar); /* rewrites the db */
  foo = g_settings_get_value (settings, "some-other-key");  /* reinits mmap /*

  /* str is now dangling */

You broke gio/tests/gsettings

Created attachment 263745 [details] [review]
g_settings_get: only check for non-copying format string

Sorry about that. Ryan suggested on irc that checking for a '&' in the format
string is enough for now.

if Ryan's ok with it then fine. (I have no opinion on how it should be fixed, I was just pointing out that "make check" was broken.)

Review of attachment 263745 [details] [review]:

Good.  Thanks.

Pushed as 0f1579e6.

Comment on attachment 263745 [details] [review]
g_settings_get: only check for non-copying format string

Updating the patch status - it was committed a while ago.