Triggered event results in crash on client disconnect

[h3ndrk]

I'm trying to get the events to work but open62541 crashes. Despite of https://github.com/open62541/open62541/issues/1995 and https://github.com/open62541/open62541/issues/1839 my example below still uses UaExpert. I've created a gist with the code at:

https://gist.github.com/NIPE-SYSTEMS/0d3aca76bab8190e044a79cc006ee63a

Code explanation

main.cpp of the gist:

• 16-115: Add event type
• 120-123: Start server thread
• 128-154: Trigger event
• 161-165: Stop server thread

There are two getchar()-calls that wait for user input. This results in: Trigger event after first Enter, shutdown after second Enter.

Reproduce crash

1. Start main.cpp
2. Start UaExpert, connect UaExpert
3. Add Server-object to "Event View" of UaExpert, enable the ErrorMessageEventType, click Apply
4. Press Enter in main.cpp (triggers event)
5. Nothing changes in UaExpert, still everything is fine, despite the missing event in UaExpert
6. Disconnect UaExpert
7. Crash happens, see call-list.md in my gist.

Interestingly when skipping the step 3 the main.cpp will not crash. It seems to have something to do with the MonitoredItems that get setup to receive the events.

I will try to observe the cause with the tips of https://github.com/open62541/open62541/issues/1997#issuecomment-415136101. Any ideas about the cause from your side?

I'm using Visual Studio 2015 on Windows 10. open62541 is at 6d868873078368171f3756819543314d6a869ba2.

Thanks in advance!

/cc @aDogCalledSpot @Pro

[h3ndrk]

I've tested one more case:

1-5: see above

6. Remove MonitoredItem from UaExpert
7. Disconnect UaExpert
8. Press Enter for the second time in main.cpp (shutdown)

This results in no crash which means that the crash only happens when the OPC UA client does not remove the subscriptions/MonitoredItems before disconnecting.

I also added Dr.Memory outputs to the gist at https://gist.github.com/NIPE-SYSTEMS/0d3aca76bab8190e044a79cc006ee63a. drmemory-without-removal.txt is the case in the first post, drmemory-with-removal.txt is the second case from this comment.

It seems odd that the client can crash the server when the client doesn't cleanup all event subscriptions.

[aDogCalledSpot]

I'm looking into the crashes. As for the events not showing in UAExpert, it is necessary to set the "Time" property of an event in order to see it in UAExpert. Furthermore, the "Text" field is added as a component but UA_Server_writeObjectProperty_scalar is being used to write to it. This call fails since it is only applicable to properties (all other fields in the event are properties so the calls work there).

[h3ndrk]

Thanks @aDogCalledSpot for your reply!

In my gist, I've fixed the property reference of the Text-field and added a new Time-field with UtcTime-type. (see 05-main.patch and 06-main.cpp) With 06-main.cpp UaExpert still displays no events.

To get UaExpert to work I've two questions:

1. Does my ErrorMessageEventType inherit the properties of the BaseEventType (e.g. Time or Severity)? Or is my approach in the gist correct: Readd the inherited properties for every new event type?
2. The properties need a TypeDefinition for UA_Server_addNode_begin(). What is the correct value for the TypeDefinition? PropertyType (i=68)? BaseDataVariableType (i=63)? UA_NODEID_NULL? I guess PropertyType...

[jpfr]

Ari, since there are mandatory fields for events, can we change the API that these fields must be set for the event creation?

I.e. take the event time as an argument.

[aDogCalledSpot]

First of all, the server crashing should be fixed in https://github.com/open62541/open62541/pull/2022.

Your problems now are closely correlated. Your own EventType will inherit all the properties from BaseEventType along with their respective QualifiedNames. This can seen quite well in the example; a new EventType inheriting from the BaseEventType is created and nothing further needs to be added. The problem you're seeing now is that the Time field is inherited from the BaseEventType and as such will keep its NamespaceIndex, which is 0. In short, you need to use 0 instead of ns for the qualified name of "Time".

[aDogCalledSpot]

mandatory fields for events

It is only mandatory for UAExpert, there is no reason the events shouldn't have an empty Time field if the time does not matter in the specific use case.

[jpfr]

Ok, understood. Can we add a paragraph to the documentation what is required for UAExpert to be happy?

Most users will be testing with it...

[h3ndrk]

Your problems now are closely correlated. Your own EventType will inherit all the properties from BaseEventType along with their respective QualifiedNames. This can seen quite well in the example; a new EventType inheriting from the BaseEventType is created and nothing further needs to be added. The problem you're seeing now is that the Time field is inherited from the BaseEventType and as such will keep its NamespaceIndex, which is 0. In short, you need to use 0 instead of ns for the qualified name of "Time".

I've removed the derived Time-property, changed the namespace and the data type from UA_TYPES_UTCTIME to UA_TYPES_DATETIME. With that UaExpert shows the events finally. Thanks for the tips!

@jpfr I've found one hint of the required Time-property in https://github.com/open62541/open62541/blob/master/examples/tutorial_server_events.c#L57. But I agree that a more prominent and detailed notice would be helpful.

@aDogCalledSpot Is it possible to default the time to 0 (1. Jan 1970) instead of delivering an empty field? Maybe we should reach out to UaExpert to ask if they can release the requirement of the Time-property? If I understand it correct, UaExpert does not follow the standard in this case but open62541 does.

[jpfr]

Is it possible to default the time to 0 (1. Jan 1970) instead of delivering an empty field?

If we set a default time at all, then it should be the current time. 99% of events will be for "now".

[Pro]

@aDogCalledSpot can you have another look at this issue?