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 661041 - Pointer corruption after modifying a string from a passed-in struct, then saving the string elsewhere
Pointer corruption after modifying a string from a passed-in struct, then sav...
Status: RESOLVED OBSOLETE
Product: vala
Classification: Core
Component: Code Generator
0.14.x
Other Linux
: Normal normal
: ---
Assigned To: Vala maintainers
Vala maintainers
Depends on:
Blocks:
 
 
Reported: 2011-10-06 01:43 UTC by clinton
Modified: 2018-05-22 14:10 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description clinton 2011-10-06 01:43:03 UTC
Steps to reproduce:

1) Compile and run the following program:

/******************************************/
struct SSS
{
    public string ss;
}

class CCC
{
    public string cs;
    public CCC(SSS incoming)
    {
        incoming.ss = "test";
        this.cs = incoming.ss;
    }
}

void main()
{
    SSS thing1 = SSS();
    thing1.ss = "uncorrupted string";
    CCC thing2 = new CCC(thing1);
}
/******************************************/

...and observe the results.

Notice that a crash occurs, usually with the following error in the console:
*** glibc detected *** ./testcase3: double free or corruption (fasttop): 0x(address) ***

Note: the code compiles and runs without crashing when using valac-0.12.
Comment 1 Luca Bruno 2011-10-06 07:25:23 UTC
Thanks for the bug report.
Notice this is a programming error. You're assigning "test" to a field of an unowned struct, which will first free the old contents.
Structs are always copied when passed, so maybe you want CCC(ref SSS incoming) to change the original passed struct.
Comment 2 Adam Dingle 2011-10-06 18:09:40 UTC
Well, of course the example is somewhat contrived, but the intention wasn't actually to change the original passed struct.  Here's a simpler test case that gets to the heart of the matter:

struct SSS {
    public string str;
}

void foo(SSS s) {
    s.str = null;
}

void main()
{
    SSS s = SSS();
    s.str = "xyzzy";
    foo(s);
}

This will also crash.  When main() calls foo(), the generated code performs only a shallow copy of the struct.  This means that the caller's struct and the callee's struct point to the same string.  When the callee sets str to null, the string is freed.  Then the same string is freed again when the struct goes out of scope at the end of main(), yielding a crash.

So, is the crash a bug in this program or in Valac?  I had assumed (perhaps naively) that when any struct is passed by value, Vala will copy all strings in the struct, so that the callee can safely modify string pointers in its copy of the struct.  Is this not the case?
Comment 3 Luca Bruno 2011-10-06 19:14:59 UTC
(In reply to comment #2)
> Well, of course the example is somewhat contrived, but the intention wasn't
> actually to change the original passed struct.  Here's a simpler test case that
> gets to the heart of the matter:
> 
> struct SSS {
>     public string str;
> }
> 
> void foo(SSS s) {
>     s.str = null;
> }
> 
> void main()
> {
>     SSS s = SSS();
>     s.str = "xyzzy";
>     foo(s);
> }
> 
> This will also crash.  When main() calls foo(), the generated code performs
> only a shallow copy of the struct.

It's exactly the same example as the previous one. It is supposed to do a shallow copy. Vala should somewhat error out, though it's something really difficult in this context.

> So, is the crash a bug in this program or in Valac?  I had assumed (perhaps
> naively) that when any struct is passed by value, Vala will copy all strings in
> the struct, so that the callee can safely modify string pointers in its copy of
> the struct.  Is this not the case?

You must use an owned parameter.
Comment 4 Jim Nelson 2011-10-06 23:34:20 UTC
Actually, in Vala 0.12, structs were *not* copied when passed:

	SSS s = {0};
	gchar* _tmp0_;
	memset (&s, 0, sizeof (SSS));
	_tmp0_ = g_strdup ("xyzzy");
	_g_free0 (s.str);
	s.str = _tmp0_;
	foo (&s);
	sss_destroy (&s);

Note the foo(&s) line.  No copy is made prior to passing, and no copy is performed in foo().

In Vala 0.14, a shallow copy is made prior to passing, meaning all pointers inside the struct are now doubly-referenced:

	SSS s = {0};
	gchar* _tmp0_;
*	SSS _tmp1_;
	memset (&s, 0, sizeof (SSS));
	_tmp0_ = g_strdup ("xyzzy");
	_g_free0 (s.str);
	s.str = _tmp0_;
*	_tmp1_ = s;
*	foo (&_tmp1_);
	sss_destroy (&s);

(Interesting lines asterisked.)  Here's foo, which is practically the same in both 0.12 and 0.14:

	gchar* _tmp0_;
	_tmp0_ = g_strdup ("abc");
	_g_free0 ((*s).str);
	(*s).str = _tmp0_;

(0.14 includes a null argument guard, but that's irrelevant here.)  The code in Adam's comment sets str to NULL, but here I had Vala set it to another string.

Note that because in Vala 0.14 the shallow-copied struct is never destroyed, the duplicated "abc" is never freed.  Hence, this new behavior introduces a memory leak.

I find this new behavior troubling for a lot of reasons.  First, it's not like the above Vala code is using pointers, which are marked as "use at own risk" in the Vala documentation.  What this new behavior says is, using a standard data type with the default ownership conferral is dangerous.  It seems to me that a compiler warning is required in the least.

Second, it's not symmetric with other data type's behaviors, specifically strings and arrays.  Neither a string nor an array are copied before passed as unowned variables.  This means that an unowned array and string may be modified by the called method (the string via its data member) but not a struct:

struct SSS {
    int a;
}

void foo(SSS s) {
    s.a = 1;
}

void main() {
    SSS s = SSS();
    s.a = 0;
    foo(s);
    stdout.printf("%d\n", s.a);
}

In Vala 0.12, the program prints one.  In Vala 0.14, the program prints zero.

In short, this is not how it's always been and we're simply tripping on our own shoelaces.

I propose this change get some serious reconsideration, because of its asymmetric behavioral changes, the way it casually introduces double-frees without warning, and because of the memory leak it introduces.
Comment 5 Jim Nelson 2012-02-21 23:06:21 UTC
Is there any movement on this ticket?  The code Adam posted in Comment 2 still crashes in trunk (0.15.1.37-d2de1) due to a double-free.

My position on this bug remains unchanged.  Structs in Vala are seriously broken without some kind of change.
Comment 6 Luca Bruno 2012-02-22 07:18:44 UTC
(In reply to comment #5)
> Is there any movement on this ticket?  The code Adam posted in Comment 2 still
> crashes in trunk (0.15.1.37-d2de1) due to a double-free.
> 
> My position on this bug remains unchanged.  Structs in Vala are seriously
> broken without some kind of change.

As said, it's a programming error. The fact that structs are now shallow copied is intentional. Use an owned parameter instead if you want a full copy.
Comment 7 GNOME Infrastructure Team 2018-05-22 14:10:53 UTC
-- 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/vala/issues/237.