Created attachment 217978 [details] [review]
gi: Fix issues with functions that modify their input parameter, like g_mkdtemp

We cannot free functions that consume an argument they take, which
is true for (transfer full) arguments.

Created attachment 217979 [details] [review]
gimarshallingtests: Add a g_mkdtemp lookalike

the gimarshallingtests side of things

Confirming:

$ jhbuild run python3 -c 'from gi.repository import GLib; a="foo.XXXXXX"; print("result:", GLib.mkdtemp(a)); print("a:", a)'
Traceback (most recent call last):

+ Trace 230498

This is because g_mkdtemp() and _full()'s annotation of tmpl

        <parameter name="tmpl" transfer-ownership="none">
          <doc xml:whitespace="preserve">template directory name</doc>
          <type name="filename" c:type="gchar*"/>
        </parameter>

which is (transfer none) does not match the return value, which is (transfer full):

     <return-value transfer-ownership="full">
        <doc xml:whitespace="preserve">A pointer to @tmpl, which has been modified to hold the directory name.  In case of errors, %NULL is returned and %errno will be set.</doc>
        <type name="utf8" c:type="gchar*"/>
      </return-value>

tmpl is also slightly confusing because it actually behaves like (inout).

If I annotatate the argument with (inout), regardless of transfer none or full, the call stops segfaulting, but neither creates a temporary dir nor returns the answer:

$ jhbuild run python3 -c 'from gi.repository import GLib; a="foo.XXXXXX"; print("result:", GLib.mkdtemp(a)); print("a:", a)'
result: (None, 'foo.XXXXXX')
a: foo.XXXXXX


But even with your patch applied, I still get the same crash, because in current g_mkdtemp(), the transfer mode of the argument is really "none". Your g-i test case annotates it as "full", but that's neither "like g_mkdtemp" nor true because g_mkdtemp() does not allocate the returned value, but simply uses the existing string. So "none" is actually correct here. But it does not matter much, if I annotate both tmpl and return value as "full" it behaves exactly the same: no crash, but no directory created and the return value is still "XXXXXX".

I'm afraid we do not currently have a good way to annotate "modifies this parameter", but it seems to me that (inout) (transfer none) would be pretty close to reality?

Comment on attachment 217978 [details] [review]
gi: Fix issues with functions that modify their input parameter, like g_mkdtemp

This does not actually fix g_mkdtemp().

Applying the patch in this bug makes it work fine:

https://bugzilla.gnome.org/show_bug.cgi?id=679351

But the patch was rejected, for sane reasons, especially because we have good functions, like g_dir_make_tmp

(In reply to comment #5)
> Applying the patch in this bug makes it work fine:
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=679351

Ah, that's essentially what I tried as well. But the concern in this bug still stands: there is no way to annotate an API that modifies its input char* argument without being (transfer full).