Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array in method result structure serialized/padded wrongly #3513

Open
4 of 7 tasks
rhaberkorn opened this issue Mar 17, 2020 · 13 comments
Open
4 of 7 tasks

Array in method result structure serialized/padded wrongly #3513

rhaberkorn opened this issue Mar 17, 2020 · 13 comments
Labels
Priority: High Issue with high priority Type: Bug Bug in the code which needs to be fixed

Comments

@rhaberkorn
Copy link

rhaberkorn commented Mar 17, 2020

Description

Implementing an AutoID server using open62541, I came across problems with arrays as part of structures returned by the Scan() method. They are apparently wrongly serialized in the binary protocol.

The Scan() method returns RfidScanResult (cf. OPC UA for AutoID, p.24), which in turn may contain several RfidSighting (p.45) structures in an array. Or as expressed in the BSD file:

 <opc:StructuredType BaseType="tns:ScanResult" Name="RfidScanResult">
  <!-- NOTE: This is a workaround for the missing opc:Bit type - probably irrelevant here -->
  <!--opc:Field TypeName="opc:Bit" Name="LocationSpecified"/-->
  <!--opc:Field Length="31" TypeName="opc:Bit" Name="Reserved1"/-->
  <opc:Field TypeName="opc:UInt32" Name="Bits"/>
  <opc:Field SourceType="tns:ScanResult" TypeName="opc:CharArray" Name="CodeType"/>
  <opc:Field SourceType="tns:ScanResult" TypeName="tns:ScanData" Name="ScanData"/>
  <opc:Field SourceType="tns:ScanResult" TypeName="opc:DateTime" Name="Timestamp"/>
  <opc:Field SwitchField="LocationSpecified" SourceType="tns:ScanResult" TypeName="tns:Location" Name="Location"/>
  <opc:Field TypeName="opc:Int32" Name="NoOfSighting"/>
  <opc:Field LengthField="NoOfSighting" TypeName="tns:RfidSighting" Name="Sighting"/>
 </opc:StructuredType>

The schema generator generates an UA_RfidScanResult accordingly:

typedef struct {
    UA_UInt32 bits;
    UA_String codeType;
    UA_ScanData scanData;
    UA_DateTime timestamp;
    UA_Location location;
    size_t sightingSize;
    UA_RfidSighting *sighting;
} UA_RfidScanResult;

The bug manifests whenever I add more than one RfidSighting to the RfidScanResult. The result you can see in this UaExpert screenshot:
UaExpert screenshot
The second sighting is apparently bogus. I've debugged the structure's contents of course - it's still perfectly fine when I store it into the output Variant. Neither does Valgrind show any kind memory corruption or the like.

I rather suspect a kind of padding issue after analyzing the network communication. You can refer to the attached Wireshark log in packet 13. The first RfidSighting ends in byte 00e7. But the second RfidSighting only starts at byte 00ec. There seem to be 4 padding bytes that are not accounted for and that are not expected by UaExpert's OPC UA stack. These 4 bytes are also present after the second RfidSighting instance.

This of course immediately raises the possibility of a 64-bit-only bug.
I haven't yet tried to recompile everything with -m32.

Background Information / Reproduction Steps

Used CMake options:

BUILD_SHARED_LIBS:BOOL=ON
CLANG_FORMAT_EXE:FILEPATH=CLANG_FORMAT_EXE-NOTFOUND
CMAKE_BUILD_TYPE:STRING=Debug
CMAKE_INSTALL_PREFIX:PATH=/usr/local
UA_ARCHITECTURE:STRING=posix
UA_BUILD_EXAMPLES:BOOL=OFF
UA_BUILD_TOOLS:BOOL=ON
UA_BUILD_UNIT_TESTS:BOOL=OFF
UA_ENABLE_AMALGAMATION:BOOL=OFF
UA_ENABLE_DISCOVERY:BOOL=ON
UA_ENABLE_DISCOVERY_MULTICAST:BOOL=OFF
UA_ENABLE_ENCRYPTION:BOOL=ON
UA_ENABLE_HISTORIZING:BOOL=OFF
UA_ENABLE_METHODCALLS:BOOL=ON
UA_ENABLE_MICRO_EMB_DEV_PROFILE:BOOL=OFF
UA_ENABLE_NODEMANAGEMENT:BOOL=ON
UA_ENABLE_SUBSCRIPTIONS:BOOL=ON
UA_ENABLE_SUBSCRIPTIONS_ALARMS_CONDITIONS:BOOL=OFF
UA_ENABLE_SUBSCRIPTIONS_EVENTS:BOOL=ON
UA_ENABLE_WEBSOCKET_SERVER:BOOL=OFF
UA_LOGLEVEL:STRING=100
UA_MULTITHREADING:STRING=0
UA_NAMESPACE_ZERO:STRING=FULL

Checklist

Please provide the following information:

  • open62541 Version (release number or git tag): v1.0.1
  • Other OPC UA SDKs used (client or server): UaExpert 1.5.1 331
  • Operating system: Linux 4.15.0-88-generic createSubscription service #88~16.04.1-Ubuntu SMP Wed Feb 12 04:19:15 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
  • Logs (with UA_LOGLEVEL set as low as necessary) attached
  • Wireshark network dump attached
  • Self-contained code example attached
  • Critical issue
@jpfr
Copy link
Member

jpfr commented Mar 17, 2020

I just added a test to verify chunking on v1.0.1. See #3515.
It appears to be fine for arrays of integer type.

Can you make a source dump available on a private channel for easier debugging?
[email protected]

@jpfr
Copy link
Member

jpfr commented Mar 17, 2020

Ah right. This is not a chunking issue, as the messages are rather small.

In addition to the UA_RfidScanResult structure and its members, there are UA_DataType structures generated by the NodeSet compiler. Can you just share these?

@rhaberkorn
Copy link
Author

Of course, I can!

I think it's easiest to upload the entire folder of files generated from the standard namespaces: src_generated.tar.gz

I've looked into types_autoid_generated.c before but couldn't figure out what's wrong.

@jpfr
Copy link
Member

jpfr commented Mar 17, 2020

Found it. The Companion Spec makes us of structures with optional fields.

This feature has just been completed.
Will be merged in #3503 in time for the v1.1.

Let's get back to this case when the infrastructure is in place.

@jpfr
Copy link
Member

jpfr commented Mar 17, 2020

General note: The nodeset compiler should abort in such cases instead of generating incomplete representations.

@rhaberkorn
Copy link
Author

General note: The nodeset compiler should abort in such cases instead of generating incomplete representations.

I don't quite understand. RfidSighting doesn't contain any optional fields and I removed all of them from RfidScanResult making all fields mandatory instead... or so I thought. Although I haven't touched Opc.Ua.AutoID.NodeSet2.xml. However, I wouldn't be surprised as I haven't studied these beasts in depth.
(And I don't want to - it's a horribly over-engineered and redundant modelling language. The BSD/CSV/NodeSet2.xml are not even meant to be manually edited AFAIK, being intermediate languages instead...)

If you can give me a quick pointer on what to change in the NodeSet2, that would be appreciated. Otherwise I'll just wait for v1.1 and hope that the generator will process it without modifications. Finding a workaround is not super urgent for me.

@jpfr
Copy link
Member

jpfr commented May 8, 2020

A few days ago, support for optional fields in structures has landed.
Can you try with the current master?

@rhaberkorn
Copy link
Author

I tried upgrading to open62541 v1.1-rc1 which was identical to master at the time of writing.

Using the ua-nodeset schemas that open62541 includes as a submodule (branch: v1.04), the schema generator throws the following error on the AutoID schema (v1.01):

Traceback (most recent call last):
  File "/usr/local/share/open62541/tools/generate_datatypes.py", line 86, in <module>
    generator.write_definitions()
  File "/usr/local/share/open62541/tools/nodeset_compiler/backend_open62541_typedefinitions.py", line 319, in write_definitions
    self.print_description_array()
  File "/usr/local/share/open62541/tools/nodeset_compiler/backend_open62541_typedefinitions.py", line 450, in print_description_array
    self.printc(self.print_datatype(t) + ",")
  File "/usr/local/share/open62541/tools/nodeset_compiler/backend_open62541_typedefinitions.py", line 133, in print_datatype
    raise RuntimeError("NodeId for " + datatype.name + " not found in .csv file")
RuntimeError: NodeId for RfidAccessResult not found in .csv file

Indeed the CSV doesn't seem to be up to date and contains no entry for RfidAccessResult. I also didn't know that AutoID v1.01 has been released. Is this a work in progress?

I went back to the ua-nodeset's master-branch schemas (AutoID v1.00), which result in a few warnings (even errors) but generate successfully at least:

INFO:__main__:Preprocessing (existing) /usr/local/share/open62541/tools/ua-nodeset/Schema/Opc.Ua.NodeSet2.xml
INFO:__main__:Preprocessing (existing) /home/rhaberkorn/metratec/pulsarlr-autoid/schemas/Opc.Ua.Di.NodeSet2.xml
INFO:__main__:Preprocessing /home/rhaberkorn/metratec/pulsarlr-autoid/schemas/Opc.Ua.AutoID.NodeSet2.xml
WARNING:nodes:Unknown Field Attribute IsOptional
WARNING:nodes:Unknown Field Attribute IsOptional
WARNING:nodes:Unknown Field Attribute IsOptional
WARNING:nodes:Unknown Field Attribute IsOptional
WARNING:nodes:Unknown Field Attribute IsOptional
WARNING:nodes:Unknown Field Attribute IsOptional
WARNING:nodes:Unknown Field Attribute IsOptional
ERROR:datatypes:ns=2;i=6128: Expected XML element with tag NMEA but found SwitchField instead
ERROR:datatypes:ns=2;i=6128: Could not parse <Body> for ExtensionObject. 'NoneType' object has no attribute 'localName'
ERROR:datatypes:ns=2;i=6147: Expected XML element with tag Grade but found EncodingMask instead
ERROR:datatypes:ns=2;i=6147: Expected Field 'Position.PositionX' not found in ExtensionObject value
ERROR:datatypes:ns=2;i=6147: Expected Field 'Position.PositionY' not found in ExtensionObject value
ERROR:datatypes:ns=2;i=6147: Expected Field 'Position.SizeX' not found in ExtensionObject value
ERROR:datatypes:ns=2;i=6147: Expected Field 'Position.SizeY' not found in ExtensionObject value
ERROR:datatypes:ns=2;i=6147: Expected Field 'Position.Rotation' not found in ExtensionObject value
ERROR:datatypes:ns=2;i=6147: Expected XML element with tag Symbology but found ScanData instead
ERROR:datatypes:ns=2;i=6147: Expected XML element with tag ImageId but found Timestamp instead
INFO:__main__:Generating Code for Backend: open62541
INFO:__main__:NodeSet generation code successfully printed

Using these (master branch) schemas, I can get my project to compile and I can also browse my device node, but all of the methods' input and output arguments seem to refer to wrong namespace ids.

Namespaces ids assigned by the schema generator (CMake calls):

ua_generate_nodeset_and_datatypes(
	NAME "di"
	FILE_CSV "${CMAKE_SOURCE_DIR}/schemas/OpcUaDiModel.csv"
	FILE_BSD "${CMAKE_SOURCE_DIR}/schemas/Opc.Ua.Di.Types.bsd"
	NAMESPACE_IDX 2
	FILE_NS "${CMAKE_SOURCE_DIR}/schemas/Opc.Ua.Di.NodeSet2.xml")

ua_generate_nodeset_and_datatypes(
	NAME "autoid"
	FILE_CSV "${CMAKE_SOURCE_DIR}/schemas/Opc.Ua.AutoID.NodeIds.csv"
	FILE_BSD "${CMAKE_SOURCE_DIR}/schemas/Opc.Ua.AutoID.Types.bsd"
	NAMESPACE_IDX 3
	FILE_NS "${CMAKE_SOURCE_DIR}/schemas/Opc.Ua.AutoID.NodeSet2.xml"
	DEPENDS "di")

Consequently all of my namespace ids are as follows:

  1. http:https://opcfoundation.org/UA/
  2. urn:open62541.server.application (Application namespace)
  3. http:https://opcfoundation.org/UA/DI/
  4. http:https://opcfoundation.org/UA/AutoID/

However, already below the Server object the ordering seems to be different (UaExpert screenshot):

2020-05-12-233255_320x166_scrot

I don't suspect this to be problematic, as my working server based on open62541 v1.0 had the same ordering when browsing these nodes.

Browsing the InputArguments of the Scan method, the only argument (of type ScanSettings) refers to NamespaceIndex 2:

2020-05-12-233530_289x126_scrot

I think that should be 3 instead according to the order previously established.
Trying to call the method (ie. opening the call dialog) from UaExpert causes all kinds of errors, related to not finding the nodes in namespace 2:

2020-05-12-233953_768x183_scrot

Consequently, I cannot test whether the serialization bug, I initially opened this ticket for, is now fixed in v1.1-rc1. Neither can I test whether optional fields and unions work now. But I did already adapt my code for them.

Needless to say, the tweaked schemas I used until now for my project cause exactly the same bug on open62541 v1.1-rc1 - the schema differences are purely cosmetic, so I won't go into detail.

I also tried to changing the NAMESPACE_IDX of AutoID to 2 (and DI to 3), hoping that things would fall together again. This fails on startup of the server as follows:

[2020-05-12 23:54:27.076 (UTC+0200)] info/session	Connection 0 | SecureChannel 0 | Session g=00000001-0000-0000-0000-000000000000 | AddNodes: The value of ns=3;i=6128 is incompatible with the variable definition
[2020-05-12 23:54:27.076 (UTC+0200)] info/session	Connection 0 | SecureChannel 0 | Session g=00000001-0000-0000-0000-000000000000 | AddNodes: Type-checking the variable node ns=3;i=6128 failed with error code BadTypeMismatch
[2020-05-12 23:54:27.076 (UTC+0200)] error/server	Adding the AutoID namespace failed. Please check previous error output.

On open62541 v1.0, where method-calling was at least working, the namespace mapping is as expected:

2020-05-12-235012_290x128_scrot

I suspect that a bug in the schema generator is to blame for this.

@rhaberkorn
Copy link
Author

PS: In both v1.0 and v1.1-rc1, the autoid_nodeids.h are unusable since they contain duplicate and conflicting macro definitions. I worked around this by manually picking the right the defintions into my source file.

@rhaberkorn
Copy link
Author

I suspect that lines like that in namespace_autoid_generated.c are responsible:

variablenode_ns_2_i_6052_variant_DataContents[0].dataType = UA_NODEID_NUMERIC(ns[1], 3010);

It should read - still did in files generated by v1.0 - ns[2]. In fact, when I manually tweak these autogenerated files, the namespaces are correct and the method InputArguments point to the right namespace.

Unfortunately, I cannot isolate the code in nodeset_compiler responsible for generating these faulty lines, nor do I see any obvious change in nodeset_compiler since v1.0 that could explain the difference.

@jpfr
Copy link
Member

jpfr commented May 21, 2020

@ichrispa would you have a look at the namespace compiler?

@apisapiaDL
Copy link

Hello,
I had the same problem with the wrong namespace in the input and output arguments. It is described in #3533

Thank you

@jpfr jpfr added Type: Bug Bug in the code which needs to be fixed Priority: High Issue with high priority labels Jul 2, 2020
@jpfr
Copy link
Member

jpfr commented Jul 2, 2020

Okay. Giving this a high priority.
@ichrispa are you aware of this one in the nodeset compiler?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High Issue with high priority Type: Bug Bug in the code which needs to be fixed
Projects
None yet
Development

No branches or pull requests

3 participants