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 761307 - Double free of elements from (unowned string)[] literal
Double free of elements from (unowned string)[] literal
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: Arrays
0.30.x
Other Linux
: Normal normal
: ---
Assigned To: Vala maintainers
Vala maintainers
Depends on:
Blocks:
 
 
Reported: 2016-01-29 20:09 UTC by Simon Lanzmich
Modified: 2016-09-16 15:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
removing the suspicious line (426 bytes, patch)
2016-06-15 03:52 UTC, Rui
none Details | Review
vala: Don't force array-elements to be owned, unowned ones are supported (2.03 KB, patch)
2016-09-16 15:02 UTC, Rico Tzschichholz
committed Details | Review

Description Simon Lanzmich 2016-01-29 20:09:14 UTC
Passing an (unowned string) array literal to a function causes the array elements to be freed afterwards, even though they should not be. This causes the array elements to be double freed. Assigning the literal to an intermediate variable and passing that to the function does not show that error.

The code below illustrates the problem.

----- example code start -----
void do_something((unowned string)[] str) {
}

void test1() {
    string s1 = "ABC", s2 = "CDE";
    (unowned string)[] s = {s1, s2};
    do_something(s);
}

void test2() {
    string s1 = "ABC", s2 = "CDE";
    do_something({s1, s2});
}

int main(string[] args) {
    test1();
    test2();
    return 0;
}
----- example code end -----

The two functions test1() and test2() should compile to equivalent C code. test1() produces correct code, whereas whereas the elements of the string array are double freed in test2().
Comment 1 Rui 2016-06-15 03:52:07 UTC
Created attachment 329832 [details] [review]
removing the suspicious line

In Vala.ArrayCreationExpression, the type of the element is always considered "ownened".

I don't know why the code says so. Removing this line seems to solve the bug. Since I only took a glance at Vala's source code, I don't know if there would be any side-effect. And I have problems running tests on my machine (tests failed to build on MinGW).

However the output from --dump-tree seems inconsistant:
  do_something({"a", "b"});
is still
  do_something(new string[2] {"a", "b"});
It really should be
  do_something(new (unowned string)[2] {"a", "b"});
Comment 2 Al Thomas 2016-06-15 17:15:54 UTC
You may be on the right track with this patch. Please note that git formatted patches are preferred.

The (unowned type)[] syntax was introduced in https://bugzilla.gnome.org/show_bug.cgi?id=571486

There is the original patch and a few later ones that can be seen here:
https://git.gnome.org/browse/vala/log/?qt=grep&q=unowned+type
Some of those patches introduce tests as well. Does make check work with your patch?

None of those patches appear to touch the file vala/valaarraycreationexpression.vala Looking at the test for https://git.gnome.org/browse/vala/commit/?id=0ffe72386589ad6a7f52f95a68929277a31cf719 it looks as though a few more tests are needed.

Are you using MSYS2 for MinGW? I think that is the most up to date set of tools.
Comment 3 Rui 2016-06-15 17:38:25 UTC
Yes, I am using MSYS2. However, make check fails with the following message:

vala-0.32.0/tests/_test/main.c:15:34: fatal error: gio/gunixinputstream.h: No such file or directory
Comment 4 Al Thomas 2016-06-15 18:12:26 UTC
GIO is part of GLib. GLib is split into a number of components, for example GObject and GIO. I've had a quick look at the MSYS2 repository ( https://github.com/Alexpux/MSYS2-packages ) and can only see GLib. So I assume everything is packaged as one. This is also how it is done on my system - Fedora Linux. 

It looks as though you may have to pass "-X -mwindows" to Valac. I'm not sure if the build system does that. The header file in the error message is for Unix, so there should be an equivalent for Windows. These are usually selected automatically based on an environment variable. I got the flags from this post: http://journeyasprogrammer.blogspot.co.uk/2014/09/install-vala-with-gtk-3-in-windows.html?showComment=1454685621822#c4107470545093286556

I also asked on IRC and there may be some D-BUS tests that will fail on Windows. I don't think that is related to your current error message though.
Comment 5 Al Thomas 2016-06-15 18:46:31 UTC
OK, looking into your make check problem further, it is the D-Bus test. UnixInputSocket is used in dbus/filedescriptor.test. If you remove the D-Bus tests it should then allow the test suite to run tests that will still cover the code that your patch applies to.
Comment 6 Rui 2016-06-15 19:39:19 UTC
Thanks for the information.

After removing all dbus-related tests now I have all other 184 tests passed.

Besides D-Bus there is also a MSYS2-specific issue: It's necessary to run

$ MSYS2_ARG_CONV_EXCL="*" make check

so that MSYS will not try to convert $testpath into some other stuff.
Comment 7 Rico Tzschichholz 2016-09-16 15:02:42 UTC
Created attachment 335710 [details] [review]
vala: Don't force array-elements to be owned, unowned ones are supported

Passing an (unowned string) array literal to a function causes the array
elements to be freed afterwards, even though they should not be. This
causes the array elements to be double freed. While assigning the literal
to an intermediate variable and passing that to the function masks this
error of the ArrayCreationExpression.
Comment 8 Rico Tzschichholz 2016-09-16 15:04:57 UTC
Attachment 335710 [details] pushed as 2ee9d3f - vala: Don't force array-elements to be owned, unowned ones are supported