Possible crash when reading multiple attributes from DeviceImpl or Device_2Impl

[mliszcz]

I've found this suspicious piece of code during #511 investigation. Reference att is re-assigned which triggers (auto-generated) copy assignment operator (this is incorrect, as Attribute allocates and owns some pointers): https://github.com/tango-controls/cppTango/blob/4d2ad6555fd6bc6e6603406ea83b6ed0ed797eb3/cppapi/server/device.cpp#L2874-L2886

I tried to trigger this code in a test (which may be wrong because it calls DeviceImpl method on Device_5Impl but I don't have any DeviceImpl available at the moment) :

    void set_throw_from_short_attr_read(bool should_throw)
    {
        DeviceData data;
        std::vector<short> value = {0, should_throw ? 1 : 0};
        data << value;
        device1->command_inout("IOAttrThrowEx", data);
    }

    void test_multiple_attributes_read_in_one_call_one_attr_throws()
    {
        set_throw_from_short_attr_read(true);

        AttributeValueList_var result;

        DevVarStringArray attribute_names;
        std::vector<std::string> attribute_names_value = {"Long_attr", "Short_attr"};
        attribute_names << attribute_names_value;

        TS_ASSERT_THROWS(result = device1->get_device()->read_attributes(attribute_names), Tango::DevFailed);

        set_throw_from_short_attr_read(false);

        TS_ASSERT_THROWS_NOTHING(result = device1->get_device()->read_attributes(attribute_names));
        TS_ASSERT_EQUALS(2u, result->length());
        TS_ASSERT_EQUALS("Long_attr", std::string(result[0].name));
        TS_ASSERT_EQUALS("Short_attr", std::string(result[1].name));
    }

diff --git a/cpp_test_suite/cpp_test_ds/DevTest.cpp b/cpp_test_suite/cpp_test_ds/DevTest.cpp
index e3a89a6d..748361e1 100644
--- a/cpp_test_suite/cpp_test_ds/DevTest.cpp
+++ b/cpp_test_suite/cpp_test_ds/DevTest.cpp
@@ -1273,9 +1273,6 @@ void DevTest::read_Short_attr(Tango::Attribute &att)
                }
        else
        {
-               Tango::Except::throw_exception((const char *)"aaa",
-                                      (const char *)"This is a test",
-                              (const char *)"DevTest::read_attr");
        }
 }

When I'm trying to read "Short_attr" (which is VALID but has no value) an exception is reported for different attribute ("Long_attr") and is followed by a segmentation fault:

Thread 9 "DevTest" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fb80e7fc700 (LWP 28343)]
_int_free (have_lock=0, p=0x7fb8000058d0, av=0x7fb800000020) at malloc.c:4280
4280    malloc.c: No such file or directory.
+(gdb) bt
#0  _int_free (have_lock=0, p=0x7fb8000058d0, av=0x7fb800000020) at malloc.c:4280
#1  __GI___libc_free (mem=0x7fb8000058e0) at malloc.c:3124
#2  0x00005587502c48eb in _CORBA_String_helper::dealloc (s=0x7fb8000058e0 "API_PollObjNotFound") at /usr/include/omniORB4/stringtypes.h:53
#3  0x00005587502c4973 in _CORBA_String_member::~_CORBA_String_member (this=0x7fb800002fd8, __in_chrg=<optimized out>) at /usr/include/omniORB4/stringtypes.h:201
#4  0x00005587502c91ac in Tango::DevError::~DevError (this=0x7fb800002fd8, __in_chrg=<optimized out>) at /home/michal/Documents/cppTango/cppapi/server/idl/tango.h:2243
#5  0x00005587502c91f6 in _CORBA_Sequence<Tango::DevError>::freebuf (b=0x7fb800002fd8) at /usr/include/omniORB4/seqTemplatedecls.h:89
#6  0x00005587502c8f45 in _CORBA_Sequence<Tango::DevError>::~_CORBA_Sequence (this=0x7fb800006710, __in_chrg=<optimized out>) at /usr/include/omniORB4/seqTemplatedecls.h:127
#7  0x00005587502c8d9e in _CORBA_Unbounded_Sequence<Tango::DevError>::~_CORBA_Unbounded_Sequence (this=0x7fb800006710, __in_chrg=<optimized out>) at /usr/include/omniORB4/seqTemplatedecls.h:253
#8  0x00005587502c87e8 in Tango::DevErrorList::~DevErrorList (this=0x7fb800006710, __in_chrg=<optimized out>) at /home/michal/Documents/cppTango/cppapi/server/idl/tango.h:2271
#9  0x00007fb81a1ad024 in Tango::DevFailed::~DevFailed (this=0x7fb8000066f0, __in_chrg=<optimized out>) at /home/michal/Documents/cppTango/cppapi/server/idl/tangoSK.cpp:265
#10 0x00007fb819293c6f in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#11 0x00007fb81a25b962 in Tango::DeviceImpl::read_attributes (this=0x5587519d8e90, names=...) at /home/michal/Documents/cppTango/cppapi/server/device.cpp:2909
#12 0x00007fb81a1b371b in _0RL_lcfn_6fe2f94a21a10053_e0000000 (cd=0x7fb80e7fb1f0, svnt=0x5587519d9ce8) at /home/michal/Documents/cppTango/cppapi/server/idl/tangoSK.cpp:2162
#13 0x00007fb819885e64 in omniCallHandle::upcall(omniServant*, omniCallDescriptor&) () from /usr/lib/x86_64-linux-gnu/libomniORB4.so.2
+(gdb) f 11
#11 0x00007fb81a25b962 in Tango::DeviceImpl::read_attributes (this=0x5587519d8e90, names=...) at /home/michal/Documents/cppTango/cppapi/server/device.cpp:2909
2909                        catch (Tango::DevFailed &)
+(gdb) p att.name
$1 = "Long_attr"

The test passes when I remove these two lines: https://github.com/tango-controls/cppTango/blob/4d2ad6555fd6bc6e6603406ea83b6ed0ed797eb3/cppapi/server/device.cpp#L2884-L2885

Reference corrections does not fix the crash:

diff --git a/cppapi/server/device.cpp b/cppapi/server/device.cpp
index ef21033b..a554526a 100644
--- a/cppapi/server/device.cpp
+++ b/cppapi/server/device.cpp
@@ -2881,7 +2881,7 @@ Tango::AttributeValueList *DeviceImpl::read_attributes(const Tango::DevVarString
                     delete back;
                     for (long j = 0; j < i; j++)
                     {
-                        att = dev_attr->get_attr_by_name(real_names[j]);
+                        Attribute& att = dev_attr->get_attr_by_name(real_names[j]);
                         att.delete_seq();
                     }

[bourtemb]

Thanks @mliszcz for the report. There is indeed something fishy in this part of the code. The attribute name reported in the exception is indeed wrong since att is pointing to the previous attribute in the given attribute list and att.get_name() is used in the exception. Your fix proposal (patch at the end of your issue description) seems quite natural to me. You are writing this is not fixing the crash. Did I understand well? att.name at least, should be fine with your version. The crash is quite strange. It seems to crash when destroying the DevFailed exception and when it is trying to deallocate the "API_PollObjNotFound" string (which is the reason field of the exception). I don't understand for the moment why there is a problem deallocating this string.

[mliszcz]

@bourtemb yes it is still crashing with the proposed patch. I did not spend much on investigating this. It stops crashing if I remove delete_seq call. Maybe delete_seq triggers some memory corruption which becomes visible during exception handling? Or maybe the issue is that this code is designed for Device and Device_2 while I am running it on Device_5.