[Swig-devel] Unsafe wrappers for members fields

[Vadim Zeitlin]

Hello,

Consider the following simple example:
---------------------------------- >8 --------------------------------------
% cat -n attr.i
1 %module attr
2
3 %inline %{
4 struct V {
5 explicit V(int n = 0) : n(n) { }
6
7 int n;
8 };
9
10 class C {
11 V MyValue;
12 };
13 %}
% cat test_attr.cs
public class test_attr
{
static void Main()
{
var c = new C();
c.MyValue = null;
}
}
% cat test_attr.py
import attr
c.MyValue = None
---------------------------------- >8 --------------------------------------

Compiling and running the tests in the usual way results in a run-time
crash (*not* an exception). I didn't test for the other languages, but I
think it should happen for all of them and not just C# and Python as the
member fields are represented as pointers by default and there are no
checks for null pointers in other languages typemaps neither AFAICS (it
definitely also happens for Java which is very similar to C#, as usual).

Of course, the above is simple to fix, once you know how to fix it: you
just need to use "%naturalvar C::MyValue". However naturalvar is not that
easily discoverable and I just don't believe it's a good idea for SWIG to
generate the code that crashes (albeit only when misused, but still) by
default.

Unfortunately I don't see any reasonably simple backwards compatible
solution for this. The simplest one would be to just turn on naturalvar by
default, but this is clearly not compatible as it would break any existing
wrappers relying on the typemaps for "SWIGTYPE*" being used for the fields.

But perhaps the documentation should at least be modified to strongly
advise the use of naturalvar for any new wrappers? Or are there are any
drawbacks to using it that I'm missing?


Further, using naturalvar isn't quite enough to solve the problem for me
because my real use case is not just a "real" member but a "synthetic" one
created by %attribute2 macro from attribute.i:
---------------------------------- >8 --------------------------------------
% cat -n attr.i
1 %module attr
2
3 %include "attribute.i"
4 %attribute2(C, V, MyValue, GetValue, SetValue);
5
6 %inline %{
7 struct V {
8 explicit V(int n = 0) : n(n) { }
9
10 int n;
11 };
12
13 class C {
14 public:
15 const V& GetValue() const { return v_; }
16 void SetValue(const V& v) { v_ = v; }
17
18 private:
19 V v_;
20 };
21 %}
---------------------------------- >8 --------------------------------------
and this macro creates uncompilable code when used with naturalvar. The
only version of the macro that expands into something compilable is
%attributestring, but it is not suitable for general purpose objects, which
are not copied, as strings are, and using it in the above example would
result in crashes when accessing the attribute during run-time.

So currently I use my own %attributebyref macro similar to
%attributestring but without %newobject and %typemap(newfree) from it and
with %naturalvar instead. This is enough to solve my own problem but,
again, I wonder if SWIG shouldn't provide such macro (maybe call it
%naturalattribute?) out of the box.

What do you think?
VZ

[William S Fulton]

Post by Vadim Zeitlin
Hello,
---------------------------------- >8 --------------------------------------
% cat -n attr.i
1 %module attr
2
3 %inline %{
4 struct V {
5 explicit V(int n = 0) : n(n) { }
6
7 int n;
8 };
9
10 class C {
11 V MyValue;
12 };
13 %}
% cat test_attr.cs
public class test_attr
{
static void Main()
{
var c = new C();
c.MyValue = null;
}
}
% cat test_attr.py
import attr
c.MyValue = None
---------------------------------- >8 --------------------------------------
Compiling and running the tests in the usual way results in a run-time
crash (*not* an exception). I didn't test for the other languages, but I
think it should happen for all of them and not just C# and Python as the
member fields are represented as pointers by default and there are no
checks for null pointers in other languages typemaps neither AFAICS (it
definitely also happens for Java which is very similar to C#, as usual).
Of course, the above is simple to fix, once you know how to fix it: you
just need to use "%naturalvar C::MyValue". However naturalvar is not that
easily discoverable and I just don't believe it's a good idea for SWIG to
generate the code that crashes (albeit only when misused, but still) by
default.
Unfortunately I don't see any reasonably simple backwards compatible
solution for this. The simplest one would be to just turn on naturalvar by
default, but this is clearly not compatible as it would break any existing
wrappers relying on the typemaps for "SWIGTYPE*" being used for the fields.
But perhaps the documentation should at least be modified to strongly
advise the use of naturalvar for any new wrappers? Or are there are any
drawbacks to using it that I'm missing?
Further, using naturalvar isn't quite enough to solve the problem for me
because my real use case is not just a "real" member but a "synthetic" one
---------------------------------- >8 --------------------------------------
% cat -n attr.i
1 %module attr
2
3 %include "attribute.i"
4 %attribute2(C, V, MyValue, GetValue, SetValue);
5
6 %inline %{
7 struct V {
8 explicit V(int n = 0) : n(n) { }
9
10 int n;
11 };
12
13 class C {
15 const V& GetValue() const { return v_; }
16 void SetValue(const V& v) { v_ = v; }
17
19 V v_;
20 };
21 %}
---------------------------------- >8 --------------------------------------
and this macro creates uncompilable code when used with naturalvar. The
only version of the macro that expands into something compilable is
%attributestring, but it is not suitable for general purpose objects, which
are not copied, as strings are, and using it in the above example would
result in crashes when accessing the attribute during run-time.
So currently I use my own %attributebyref macro similar to
%attributestring but without %newobject and %typemap(newfree) from it and
with %naturalvar instead. This is enough to solve my own problem but,
again, I wonder if SWIG shouldn't provide such macro (maybe call it
%naturalattribute?) out of the box.
What do you think?

Your proposals sound good. Better documentation and expanding
attribute.i functions all sound like the best solution given backwards
compatibility constraints. We just need a good name for the new macro,
or more likely family of additional macros.

William