tango_const.h possible seg fault

[alexElettra]

In include/tango/tango_const.h there are the defines:

enum CmdArgType {
DEV_VOID = 0,
DEV_BOOLEAN,
DEV_SHORT,
DEV_LONG,
DEV_FLOAT,
DEV_DOUBLE,
DEV_USHORT,
DEV_ULONG,
DEV_STRING,
DEVVAR_CHARARRAY,
DEVVAR_SHORTARRAY,
DEVVAR_LONGARRAY,
DEVVAR_FLOATARRAY,
DEVVAR_DOUBLEARRAY,
DEVVAR_USHORTARRAY,
DEVVAR_ULONGARRAY,
DEVVAR_STRINGARRAY,
DEVVAR_LONGSTRINGARRAY,
DEVVAR_DOUBLESTRINGARRAY,
DEV_STATE,
CONST_DEV_STRING,
DEVVAR_BOOLEANARRAY,
DEV_UCHAR,
DEV_LONG64,
DEV_ULONG64,
DEVVAR_LONG64ARRAY,
DEVVAR_ULONG64ARRAY,
DEV_INT,
DEV_ENCODED,
DEV_ENUM,
DEV_PIPE_BLOB,
DEVVAR_STATEARRAY,
DATA_TYPE_UNKNOWN = 100
};
and:
const char * const CmdArgTypeName[] = {
"DevVoid",
"DevBoolean",
"DevShort",
"DevLong",
"DevFloat",
"DevDouble",
"DevUShort",
"DevULong",
"DevString",
"DevVarCharArray",
"DevVarShortArray",
"DevVarLongArray",
"DevVarFloatArray",
"DevVarDoubleArray",
"DevVarUShortArray",
"DevVarULongArray",
"DevVarStringArray",
"DevVarLongStringArray",
"DevVarDoubleStringArray",
"DevState",
"ConstDevString",
"DevVarBooleanArray",
"DevUChar",
"DevLong64",
"DevULong64",
"DevVarLong64Array",
"DevVarULong64Array",
"DevInt",
"DevEncoded",
"DevEnum",
"DevPipeBlob",
"DevVarStateArray",
"Unknown"
};
BUT doing Tango::CmdArgTypeName[Tango::DATA_TYPE_UNKNOWN] an over bounds access (index = 100) on an array made of 33 elements is done.

CAREFUL in using such a syntax!

Cheers,
Alessandro

[t-b]

Good point. This was introduced in 3c7bccc.

[bourtemb]

Good point, guys!
Moreover, this CmdArgTypeName array is advertised in the documentation without specifying this risk:
https://tango-controls.readthedocs.io/en/latest/development/advanced/reference.html?highlight=CmdArgTypeName#tango-data-type

To be safe, in the current situation, one should always check the index value before accessing CmdArgTypeName array as it is done in devapi_attr.cpp in the following function:
ostream &operator<<(ostream &o_str,DeviceAttribute &da)

//
// Print data type
//

if (da.get_type() != DATA_TYPE_UNKNOWN)
	o_str << "Data type = " << CmdArgTypeName[da.get_type()] << ")" << endl;

I guess this was added because if new data types would be added in the future, DATA_TYPE_UNKNOWN would still be the last one and its value would not change.

So from here, I see the 2 following possibilities:

1. We remove " = 100" in the definition of DATA_TYPE_UNKNOWN. Could this produce a compatibility issue? It seems this was introduced when the DEV_ENUM type was added. The question is: could this DATA_TYPE_UNKNOWN type value be transferred on the network? At first sight it looks like it is not but I might be wrong.
   It would be interesting to get @taurel 's opinion on this topic to see whether he remembers whether he encountered some difficulties when he added the DEV_ENUM type before introducing this change.
2. We update the documentation and warn about this risk (The section I mentioned above seems to be the only one mentioning CmdArgTypeName). Still risky since not so many people read the doc!

[bourtemb]

Another possibility would be to use a map instead of an array for CmdArgTypeName.

[t-b]

We can introduce a convenience function for Tango 9 as in

constexpr std::string DataTypeToString(CmdArgType type)
{
    return type >= DATA_TYPE_UNKNOWN ? "Unknown" : CmdArgTypeName[type];
}

(of course using a tango style macro for constexpr).

which would handle the checking. For later versions I would convert CmdArgType to a enum class and create an output operator overload.