PVStructure::getSubField() can return NULL

[mdavidsaver]

FYI. PVStructure::getSubField() returns shared_ptr() (aka NULL) when the requested field doesn't exist. There is another variant PVStructure::getSubFieldT() (notice the highly obvious "T") which throws an exception instead. When chaining method calls ```getSubFieldT()`` is highly recommended.

ADCore/ADApp/ntndArrayConverterSrc/ntndArrayConverter.cpp

Line 78 in acd97bd

if((*it)->getSubField<PVString>("name")->get() == "ColorMode")

[MarkRivers]

I am trying to generate an error with the existing version of the code. I'm running the ADCSimDetector which generates NDArrays without the ColorMode attribute. In fact the NDArrays have no attributes at all. I then use NDPluginPva convert these to NTNDArrays which are sent to another IOC running pvaDriver. Both IOCs seem to be running fine with no errors. So although GetColorMode() should be failing I see no errors. How do I generate errors to know when I have fixed the problem?

Did you observe problems, or just notice the problem in the code?

[mdavidsaver]

So although GetColorMode() should be failing I see no errors. How do I generate errors to know when I have fixed the problem?

I also note that NTNDArray::getAttribute(), as with most NTNDArray methods, returns NULL if the "attribute" field does not exists.

ADCore/ADApp/ntndArrayConverterSrc/ntndArrayConverter.cpp

Line 73 in acd97bd

PVStructureArray::const_svector attrs(m_array->getAttribute()->view());

My guess then is that your Structure has "attributes", but that the PVStructureArray is empty.

To trigger a crash you would either have to get rid of "attributes" or change this StructureArray such that:

• "name" isn't PVString
• "value" isn't an PVUnion
• said union doesn't contain a PVInt

Really making the slightest change to the Structure definition will cause a crash.

Did you observe problems, or just notice the problem in the code?

I just noticed this. Incomplete error handling in the case of missing fields, or mis-matched types, is probably the most common gotcha of pvData.

[mdavidsaver]

If you want to experiment with different structure definitions have a look at iocBoot/iocimagedemo/ in QSRV. Removing the last two records "$(N):ColorMode_" and "$(N):ColorMode" will remove the "attributes" sub-structure.

https://github.com/epics-base/pva2pva/blob/master/iocBoot/iocimagedemo/image.db#L45

Similarly you can change the Field types by changing the record types.

[brunoseivam]

Isn't attribute a mandatory field of NTNDArray? If so, malformed NTNDArrays should be caught when wrapping them at the receiving code.

However, malformed attribute structures inside attribute's array would cause the problem you mentioned.

[MarkRivers]

@brunoseivam I have 2 IOCs that are generating NTNDArrays. ADSimDetector is generating arrays with several attributes including ColorMode, and ADCSimDetector is generating arrays with no attributes.

This is the output of pvget from ADSimDetector:

corvette:~>pvget -r 'field(attribute)' 13SIM1:Pva1:Image
13SIM1:Pva1:Image
structure
    epics:nt/NTAttribute:1.0[] attribute
        epics:nt/NTAttribute:1.0
            string name ColorMode
            any value
                int  0
            string[] tags []
            string descriptor Color mode
            int sourceType 0
            string source Driver
        epics:nt/NTAttribute:1.0
            string name CameraManufacturer
            any value
                string  Simulated detector
            string[] tags []
            string descriptor Camera manufacturer
            int sourceType 2
            string source 13SIM1:cam1:Manufacturer_RBV
        epics:nt/NTAttribute:1.0
            string name CameraModel
            any value
                string  Basic simulator
            string[] tags []
            string descriptor Camera model
            int sourceType 2
            string source 13SIM1:cam1:Model_RBV
        epics:nt/NTAttribute:1.0
            string name ImageCounter
            any value
                int  9999
            string[] tags []
            string descriptor Array counter
            int sourceType 2
            string source 13SIM1:cam1:ArrayCounter_RBV
        epics:nt/NTAttribute:1.0
            string name MaxSizeX
            any value
                int  1936
            string[] tags []
            string descriptor Detector X size
            int sourceType 2
            string source 13SIM1:cam1:MaxSizeX_RBV
        epics:nt/NTAttribute:1.0
            string name MaxSizeY
            any value
                int  1216
            string[] tags []
            string descriptor Detector Y size
            int sourceType 2
            string source 13SIM1:cam1:MaxSizeY_RBV
        epics:nt/NTAttribute:1.0
            string name BinX
            any value
                int  1
            string[] tags []
            string descriptor Binning in X
            int sourceType 2
            string source 13SIM1:cam1:BinX_RBV
        epics:nt/NTAttribute:1.0
            string name BinY
            any value
                int  1
            string[] tags []
            string descriptor Binning in Y
            int sourceType 2
            string source 13SIM1:cam1:BinY_RBV
        epics:nt/NTAttribute:1.0
            string name AcquireTimePV
            any value
                double  0.001
            string[] tags []
            string descriptor Camera acquire time
            int sourceType 2
            string source 13SIM1:cam1:AcquireTime_RBV
        epics:nt/NTAttribute:1.0
            string name AcquirePeriod
            any value
                double  0.1
            string[] tags []
            string descriptor Acquire period
            int sourceType 2
            string source 13SIM1:cam1:AcquirePeriod_RBV
        epics:nt/NTAttribute:1.0
            string name ImageMode
            any value
                short  1
            string[] tags []
            string descriptor Image mode
            int sourceType 2
            string source 13SIM1:cam1:ImageMode_RBV
        epics:nt/NTAttribute:1.0
            string name TriggerMode
            any value
                short  0
            string[] tags []
            string descriptor Trigger mode
            int sourceType 2
            string source 13SIM1:cam1:TriggerMode_RBV

This is the output from pvget with ADCSimDetector:

corvette:~>pvget -r 'field(attribute)' 13ADCSIM1:Pva1:Image
13ADCSIM1:Pva1:Image
structure
    epics:nt/NTAttribute:1.0[] attribute

So ADCSimDetector has no attributes, so there is no ColorMode attribute. But ntndArrayConverter is not generating any error. Is getColorMode() actually being called when converting NTNDArray to NDArray in pvaDriver? If so why is it not generating an error?

[brunoseivam]

getColorMode is only called from getInfo, which in turn is called from here in pvaDriver.

If the underlying attribute array is empty (but exists), getColorMode's iterator will simply not do anything and the function will return a default value (NDColorModeMono).

If the NTNDArray doesn't have the attribute field, this error will (I expect) be caught here, by the wrap method.

I think you will see an error if you purposefully generate an NTNDArray with an attribute array containing bogus structures (e.g., missing the "name" field).

[mdavidsaver]

this error will (I expect) be caught here, by the wrap method.

Ah, were it only so... Actually it will crash here due to an unchecked getField() call (also returns NULL). Did I mention that this is a common error?

https://github.com/epics-base/normativeTypesCPP/blob/master/src/ntndarray.cpp#L352

To repeat my message. If you call a getSubField() then you must check that the result is not NULL before using it. If you are certain it will never be NULL, then use getSubFieldT() so that violations of this assumption will fail in a more obvious, and less destructive, way.

[mdavidsaver]

Also, the unchecked use of static_pointer_cast is equally problematic.

ADCore/ADApp/ntndArrayConverterSrc/ntndArrayConverter.cpp

Line 81 in acd97bd

int cm = static_pointer_cast<PVInt>(field->get())->get();

What should be done here is something like

if(!*it) continue;
PVScalarPtr field((*it)->getSubFieldT<PVUnion>("value")->get<PVScalar>());
if(!field)
    continue;
colorMode = field->getAs<uint32>();

This will throw if "value" doesn't exist, or isn't a union. It will silently ignore if "value" contains a non-scalar value (array or sub-structure). It will implicitly convert if the "value" union happens to contain PVShort or PVULong. It will throw if "value" contains a PVString with a value which can't be converted to an unsigned integer by the rules of castUnsafe<>() (see pv/typeCast.h). But it should never crash.

edit: Added initial test of *it as structure array elements may be NULL.

[mdavidsaver]

FYI You should also default to using PVScalar or PVScalarArray instead of eg. PVInt or PVIntArray. This adds some robustness in the face of what should be inconsequential changes in sign and width of integer types.

As efficient handling of array data is important to AD I will point out PVScalarArray::getAs<void>() which allows access to raw array data as shared_vector<const void> w/o copying.

[brunoseivam]

Thanks for the tips, Michael! Is there any document with recommendations like this (or a more general "how to use pvData")? If not, is one planned?

[mdavidsaver]

Unfortunately no usage documentation exists yet which mentions PVScalar::getAs()/putFrom() or the void specialization of shared_vector. These are (somewhat) recent additions. It's in my mind to do this, but I don't yet have any time allocated.