Created attachment 319638 [details] [review]
arg: Don't free (transfer none) strings

The testUtf8() test (part of /js/EverythingBasic) crashes for me because
of freeing a pointer that was never allocated. (This is undefined
behavior.) It seems that under some circumstances we try to free const
gchar * return values from introspected functions.

Please review carefully, as I'm not entirely sure about this change. It
seems to work, and it seems to be correct in analogy to the surrounding
code. I checked that in the testUtf8() test the non-const return values
are still freed, and the const return values are not. However, if I am
wrong, then this could potentially cause huge memory leaks.

There was a change in the g-i test suite here, see https://git.gnome.org/browse/gobject-introspection/commit/?id=dcf87051f8ffd7b2bfc48d2e723b208c3345434c
although this seems like a different test.  Needs more investigation.

Review of attachment 319638 [details] [review]:

::: gi/arg.cpp
@@ +2958,3 @@
     case GI_TYPE_TAG_UTF8:
+        if (transfer != TRANSFER_IN_NOTHING)
+            g_free(arg->v_pointer);

Not sure about the test, but this does not seem correct.

For in arguments:

For TRANSFER_IN_NOTHING, we have allocated a new block of memory to do the UTF16-to-UTF8 conversion, and passed that to the C function. The C function is (transfer none) so it did nothing with the memory. If we don't free, we have a leak.

Conversely, for GI_TRANSFER_EVERYTHING, the C function has taken care of the memory block, so we should not free it ourselves. But for GI_TRANSFER_EVERYTHING we can't reach this function.

For out arguments:

TRANSFER_IN_NOTHING does not exist. GI_TRANSFER_NOTHING is shortcutted and unreachable. For GI_TRANSFER_EVERYTHING, we have a new block of memory that we have to free (because the JS code does not use it, it uses GC memory).

So if I read it correctly, for in and out arguments the old code was correct. The only problem is inout, which at the moment is underspecified (and fails the test).

OK, I buy that the patch is incorrect.

I would interpret inout like this: a function with an inout parameter

/* inout: (transfer full): */
void func(char **inout) {
    g_assert (strcmp (*inout, utf8_const) == 0);
    g_free (*inout);
    *inout = g_strdup (utf8_nonconst);
}

should have exactly the same transfer rules as a function with two separate parameters

/* in: (transfer full):
 * out: (transfer full): */
void func2(char *in, char **out) {
    g_assert (strcmp (in, utf8_const) == 0);
    g_free (in);
    *out = g_strdup (utf8_nonconst);
}

Unless I am misunderstanding (which I suspect I am, because I don't understand what you mean by inout is underspecified) then I think the problem is actually here: https://git.gnome.org/browse/gjs/tree/gi/function.cpp#n1104

I think your interpretation of inout is one possible, and sensible, interpretation, but not necessarily the only one.
gjs's interpretation is that

inout; (transfer full)

becomes
in: (transfer none)
out: (transfer full)

And I think compatibility with existing libraries requires this behavior, but I am not sure. Not many libraries use inout.

That's what I mean by underspecified.

Got it.

The change to the g-i test suite would seem to suggest that it has now become specified, though, as

in: (transfer full)
out: (transfer full)

If that wasn't intentional, then it should probably be reverted. If it was intentional, then probably libraries that use inout in the old way should be patched.

Where would be a good place to discuss this?

*** Bug 765465 has been marked as a duplicate of this bug. ***

This is caused by the patches committed in bug 736517

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gjs/issues/95.