GNOME Bugzilla – Bug 656693
Functions don't check null struct parameter
Last modified: 2011-08-17 16:22:54 UTC
Here is the Vala code: public struct Tool { public int a; } public void handle_tool (Tool tool) { tool.a = 24; } public static int main (string[] args) { Tool good_tool = Tool (); handle_tool (good_tool); Tool? bad_tool = null; handle_tool (bad_tool); return 0; } ---------- The handle_tool() function in C: void handle_tool (Tool* tool) { (*tool).a = 24; } So it results with a segfault for "bad_tool". Vala should add a return_if_fail() or similar in the function, since the parameter is "Tool tool", not "Tool? tool".
commit 94d4b1e91a437f739b42f0ac4021faf5e9aa664c Author: Luca Bruno <lucabru@src.gnome.org> Date: Tue Aug 16 22:37:58 2011 +0200 codegen: Assign to temp varable when passing a nullable struct argument Assign possible (*expr) to a temp variable to ensure copying the struct. Fixes bug 656693. The problem is that we want the caller to crash because the programmer must check for the struct to be null before calling the method. The commit in reality fixes this bug plus possible side-effects from directly passing the variable. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Thanks for the quick fix. Unfortunately, I think it's still broken. I've installed vala master, and the new test located at "tests/structs/bug656693.vala" fails. > The problem is that we want the caller to crash because the programmer must check for the struct to be null before calling the method. I agree with this. But a segfault can be difficult to diagnose. Also, it's not homogeneous with other parameter types. If an object is null, and if the parameter doesn't have "?", there is a return_if_fail().
(In reply to comment #2) > Thanks for the quick fix. > > Unfortunately, I think it's still broken. I've installed vala master, and the > new test located at "tests/structs/bug656693.vala" fails. Fails? Any details? > > > The problem is that we want the caller to crash because the programmer must > check for the struct to be null before calling the method. > > I agree with this. But a segfault can be difficult to diagnose. Also, it's not > homogeneous with other parameter types. If an object is null, and if the > parameter doesn't have "?", there is a return_if_fail(). We can discuss whether the function must use return_if_fail(), it might make sense from a C library view point (better open another bug for this). Still we want a crash on the caller side, a struct is not an object type by definition.
> Fails? Any details? $ /usr/local/bin/valac --version Vala 0.13.2.4-1dd6 $ /usr/local/bin/valac bug656693.vala $ ./bug656693 ** ERROR:/home/seb/programmation/vala/bug656693.vala.c:71:_vala_main: assertion failed: ((*foo).bar == 0) Aborted > We can discuss whether the function must use return_if_fail(), it might make > sense from a C library view point (better open another bug for this). Still we > want a crash on the caller side, a struct is not an object type by definition. This was the object of this bug report. I don't know what you have fixed, since the test fails, and the C code generated is unchanged. With the return_if_fail(), there is a critical message if the parameter is null. An application should never have critical messages, it's a programmer error to rely on them. It's possible to configure the compilation so the program crash on critical messages. And if it occurs, we directly know what is wrong. If there is no return_if_fail(), the code generated is exactly the same whether the parameter have a "?" or not. So without the "?", a null value is allowed. But, the following code is not allowed: func (Tool tool) { if (tool == null) // ... } It's a contradiction. So in this case, if we want to have a robust code, we should always write: func (Tool? tool) { return_if_fail (tool != null); } If a program has not been enough tested, it's better to have a critical message, than a segfault with probably data loss. And as you say, for a library, robustness is even more important.
(In reply to comment #4) > > Fails? Any details? > > $ /usr/local/bin/valac --version > Vala 0.13.2.4-1dd6 > $ /usr/local/bin/valac bug656693.vala > $ ./bug656693 > ** > ERROR:/home/seb/programmation/vala/bug656693.vala.c:71:_vala_main: assertion > failed: ((*foo).bar == 0) > Aborted > > This was the object of this bug report. I don't know what you have fixed, since > the test fails, and the C code generated is unchanged. I have the same version here and get the following generated C code: _tmp3_ = *foo; baz (&_tmp3_); Can you please paste your C code relatively to the baz() call?
void baz (Foo* foo) { (*foo).bar = 3; } void _vala_main (void) { Foo _tmp0_ = {0}; Foo _tmp1_ = {0}; Foo* _tmp2_; Foo* foo; memset (&_tmp0_, 0, sizeof (Foo)); _tmp1_ = _tmp0_; _tmp2_ = _foo_dup0 (&_tmp1_); foo = _tmp2_; baz (foo); g_assert ((*foo).bar == 0); _foo_free0 (foo); } There is no _tmp3_. Strange.
commit 54516a7eade2864c81352fd2bc8a90837297b97d Author: Luca Bruno <lucabru@src.gnome.org> Date: Wed Aug 17 17:13:57 2011 +0200 codegen: Add null check for non-null struct parameters Commit 94d4b1e91a437f739b42f fixes another relevant part of the bug. Fixes bug 656693. About that code, I even have another different code here: void _vala_main (void) { Foo _tmp0_ = {0}; Foo _tmp1_; Foo* _tmp2_; Foo* foo; gint _tmp3_; memset (&_tmp0_, 0, sizeof (Foo)); _tmp1_ = _tmp0_; _tmp2_ = _foo_dup0 (&_tmp1_); foo = _tmp2_; baz (foo); _tmp3_ = (*foo).bar; g_assert (_tmp3_ == 0); _foo_free0 (foo); } Can you ensure you're using the latest libvala? You might have installed only the valac binary.
Sorry for the noise, pasting the right code (mixed with yours somehow): void _vala_main (void) { Foo _tmp0_ = {0}; Foo _tmp1_; Foo* _tmp2_; Foo* foo; Foo _tmp3_; gint _tmp4_; memset (&_tmp0_, 0, sizeof (Foo)); _tmp1_ = _tmp0_; _tmp2_ = _foo_dup0 (&_tmp1_); foo = _tmp2_; _tmp3_ = *foo; baz (&_tmp3_); _tmp4_ = (*foo).bar; g_assert (_tmp4_ == 0); _foo_free0 (foo); }
OK, it was indeed an installation problem. Since the valac reported the good version, I thought the installation was correct. And thank you for the second commit.