Node deletion bug?

[bostjan....@gmail.com]

Hi Julius and others,

I noticed that when I delete a node, the reference from parent node to deleted node remains to exist. Is this expected behavior or is it a bug?

Best regards,

Bostjan

[Julius Pfrommer]

Hello Bostjan,

the DeleteNodes service has a flag deleteReferences that is used to select whether stray references can stay after removing the node.
That's from the OPC UA spec, not our design.

Best regards,
Julius

[bostjan....@gmail.com]

Hello Julius,

I am aware of deleteReferences flag. In particular, I am deleting a node with the following call:

UA_Server_deleteNode(server, nodeId, UA_TRUE);

Let me elaborate a little more. I am executing the following code:

1) Adding a node: UA_Server_addObjectNode(server, UA_NODEID_NUMERIC(1, 0), ..., &nodeId);

2) Deleting a node: UA_Server_deleteNode(server, nodeId, UA_TRUE);

3) Adding a node again: UA_Server_addObjectNode(server, UA_NODEID_NUMERIC(1, 0), ..., &nodeId);

When I browse the parent node using UA_Client_Service_browse or UA_Server_browse, I am seeing two references to the node in question. The same can be seen in the UaExpert's References view.

Regards,

Bostjan

[Boštjan Vesnicer]

Hello Julius,

in the meantime I figured out that if I manually delete a reference from parent node to the node in question everything works ok - after I add node the second time there is only a single reference which is what I want.

My understanding was that node deletion should delete also all the references which "point" to the deleted node (i.e. deleted node is the reference's target node) but I could be wrong on this.

Regards,

Bostjan

[Julius Pfrommer]

Hello Bostjan,

yes, with the "deleteReferences" argument enabled, the service should delete references pointing to the node.

I wrote a unit test to check for the behavior you described.
The test does however not reproduce the issue.

https://github.com/open62541/open62541/blob/0.2/tests/check_services_nodemanagement.c#L175

Maybe you can tell us how to change the unit test so that we can reproduce the error.

Thanks,
Julius

[bostjan....@gmail.com]

Hello Julius,

I've create a pull request #937. Please have a look.

Best regards,

Bostjan

[Grant Edwards]

On Monday, January 16, 2017 at 4:28:05 AM UTC-6, bostjan....@gmail.com wrote:

I noticed that when I delete a node, the reference from parent node to deleted node remains to exist. Is this expected behavior or is it a bug?

Yes, I see the same thing (I'm using 0.2).

For example, I start with node 30000 organizes nodes 80000 80001 80002 80003 (which are simple scalar variable nodes).

Then I delete the 8000x nodes with

UA_Server_deleteNode(server, nodeid, true);

When I look at the children of 30000, the references are still there, but when I call UA_Server_readDisplayName() to read the names of any of the 8000x nodes it returns status of 0x00000001.

If I add the 8000x nodes back, then there are two references to each and the display names are what they should be.

If I delete and add them back again, then there are three references to each.

I'm displaying the references using this code:

static char *nodeDisplayName(UA_NodeId *n)
{
  static char buf[4097];
  UA_LocalizedText dispname;

  UA_StatusCode s = UA_Server_readDisplayName(server, *n, &dispname);

  if (s)
    {
      sprintf(buf,"UA_Server_readDisplayName: 0x%x",s);
    }
  else
    {
      memcpy(buf,dispname.text.data,dispname.text.length);
      buf[dispname.text.length] = '\0';
    }
  return buf;
}

static char spaces[] = "                                                              ";

static UA_StatusCode printnode(UA_NodeId childId, UA_Boolean isInverse, UA_NodeId referenceTypeId, void *handle)
{
  if (isInverse)
    return 0;
   
  UA_NodeId hasTypeDefinition = UA_NODEID_NUMERIC(0, UA_NS0ID_HASTYPEDEFINITION);

  if (UA_NodeId_equal(&referenceTypeId,&hasTypeDefinition))
    return 0;

  intptr_t level = (intptr_t)handle;
  printf("%.*s%s: ",(int)level,spaces,nodeDisplayName(&referenceTypeId));
  printf("%s %s\n",nodeIdString(&childId),nodeDisplayName(&childId));
  UA_Server_forEachChildNodeCall(server, childId, printnode, Handle(level+1));
  return 0;
}


[...]

    UA_Server_forEachChildNodeCall(server,nodeid,printnode,Handle(1));

[...]

[Grant Edwards]

On Wednesday, January 18, 2017 at 5:21:14 PM UTC-6, Grant Edwards wrote:

On Monday, January 16, 2017 at 4:28:05 AM UTC-6, bostjan....@gmail.com wrote:

I noticed that when I delete a node, the reference from parent node to deleted node remains to exist. Is this expected behavior or is it a bug?

Yes, I see the same thing (I'm using 0.2).

I'm attaching server code that demonstrates the problem and the output I see when I run it (on Linux).  It builds with the single-file 0.2 distribution.

--
Grant

[Grant Edwards]

As Bostjan mentioned, manually deleting the reference seems to be a usable work-around.  Here's a diff showing that workaround applied to the demo_server.c code I previously attached:

--- server_demo.c    2017-01-18 18:17:51.689206477 -0600
+++ server_demo-workaround.c    2017-01-18 18:17:11.635853665 -0600
@@ -112,25 +112,35 @@
 }
 
 void deleteChild(UA_NodeId *n, char *name)
 {
   UA_NodeId childid = UA_NODEID_STRING(1,name);
   UA_StatusCode s = UA_Server_deleteNode(server,childid,true);
   if (s)
     printf("deleteChild: %08x\n",s);
 }
 
+void deleteReference(UA_NodeId *n, char *name)
+{
+  UA_ExpandedNodeId childid = UA_EXPANDEDNODEID_STRING(1,name);
+  UA_StatusCode s = UA_Server_deleteReference(server, *n, UA_NODEID_NUMERIC(0, UA_NS0ID_ORGANIZES), true, childid, false);
+  if (s)
+    printf("deleteReference: %08x\n",s);
+}
+
 void deleteChildren(UA_NodeId *n)
 {
   printf("Deleting children...\n");
   deleteChild(n,"the answer");
+  deleteReference(n,"the answer");
   deleteChild(n,"other answer");
+  deleteReference(n,"other answer");
 }
 
 void showChildren(UA_NodeId *n)
 {
   printf("%s:\n",nodeDisplayName(n));
   UA_Server_forEachChildNodeCall(server,*n,printnode,Handle(1));
 }
 
 
 int main(int argc, char** argv)

[Julius Pfrommer]

Fixed on master and 0.2.

This was a bug that was never uncovered in the unit tests, since the Browse service only returns references to nodes that actually exist.
So the reference disappears in the Browse service when the node is deleted. And we have 2 references instead of one when a node with the same id is added again.

Thanks for digging into this Bostjan and Grant Edwards.

Best regards,
Julius

[Boštjan Vesnicer]

Great. Thanks for fixing the bug.

Best regards,

Bostjan

[Grant Edwards]

On Thursday, January 19, 2017 at 5:22:29 AM UTC-6, Julius Pfrommer wrote:

Fixed on master and 0.2.

The dangling references are gone, but now I get valgrind errors that I didn't get before this fix.

The errors only seem to happen when I delete a node in a UA_Server_forEachChildNodeCall() callback function.

Here's the recursive delete code I'm using:

static void deleteChildren_r(UA_NodeId *nodeid);

static UA_StatusCode deleteChildrenCallback(UA_NodeId childId, UA_Boolean isInverse, UA_NodeId referenceTypeId, void *handle)
{
  if (isInverse)
    return UA_STATUSCODE_GOOD;

  UA_NodeId organizes = UA_NODEID_NUMERIC(0, UA_NS0ID_ORGANIZES);

  if (UA_NodeId_equal(&referenceTypeId,&organizes))
    {
      deleteChildren_r(&childId);
      UA_Server_deleteNode(server,childId,true);
    }
  return UA_STATUSCODE_GOOD;
}

// recursive deletion of nodes that are "orgranized" by <nodid>
static void deleteChildren_r(UA_NodeId *nodeid)
{
  UA_Server_forEachChildNodeCall(server,*nodeid,deleteChildrenCallback,NULL);
}

Before this latest fix, that did not product any valgrind errors (though it did leave dangling references).  After this morning's git pull from 0.2, I get valgrind errors like this for each node that is deleted:

deleting children...
==18655== Invalid read of size 1
==18655==    at 0x402BD0: fnv32 (ua_types.c:281)
==18655==    by 0x402C59: UA_NodeId_hash (ua_types.c:295)
==18655==    by 0x40D564: findNode (ua_nodestore.c:97)
==18655==    by 0x40DD10: UA_NodeStore_remove (ua_nodestore.c:306)
==18655==    by 0x414B15: Service_DeleteNodes_single (ua_services_nodemanagement.c:1185)
==18655==    by 0x414C40: UA_Server_deleteNode (ua_services_nodemanagement.c:1216)
==18655==    by 0x401B44: deleteChildrenCallback (server_demo.c:150)
==18655==    by 0x40516A: UA_Server_forEachChildNodeCall (ua_server.c:128)
==18655==    by 0x401B98: deleteChildren_r (server_demo.c:158)
==18655==    by 0x401BC0: deleteChildren (server_demo.c:164)
==18655==    by 0x401F07: main (server_demo.c:201)
==18655==  Address 0x5234920 is 0 bytes inside a block of size 10 free'd
==18655==    at 0x4C2D1CC: free (vg_replace_malloc.c:473)
==18655==    by 0x4022EC: String_deleteMembers (ua_types.c:85)
==18655==    by 0x4028E4: NodeId_deleteMembers (ua_types.c:194)
==18655==    by 0x402CA1: ExpandedNodeId_deleteMembers (ua_types.c:304)
==18655==    by 0x40468F: deleteMembers_noInit (ua_types.c:950)
==18655==    by 0x40472A: UA_deleteMembers (ua_types.c:964)
==18655==    by 0x41140D: UA_ReferenceNode_deleteMembers (ua_types_generated_handling.h:1474)
==18655==    by 0x414D6F: deleteOneWayReference (ua_services_nodemanagement.c:1241)
==18655==    by 0x41E2DF: UA_Server_editNode (ua_server_utils.c:262)
==18655==    by 0x414EEA: Service_DeleteReferences_single (ua_services_nodemanagement.c:1260)
==18655==    by 0x4149FA: removeReferences (ua_services_nodemanagement.c:1159)
==18655==    by 0x414AFE: Service_DeleteNodes_single (ua_services_nodemanagement.c:1183)
==18655==
==18655== Invalid read of size 1
==18655==    at 0x4C31FFC: __memcmp_sse4_1 (vg_replace_strmem.c:972)
==18655==    by 0x4022BC: UA_String_equal (ua_types.c:78)
==18655==    by 0x402B68: UA_NodeId_equal (ua_types.c:263)
==18655==    by 0x40D5D9: findNode (ua_nodestore.c:107)
==18655==    by 0x40DD10: UA_NodeStore_remove (ua_nodestore.c:306)
==18655==    by 0x414B15: Service_DeleteNodes_single (ua_services_nodemanagement.c:1185)
==18655==    by 0x414C40: UA_Server_deleteNode (ua_services_nodemanagement.c:1216)
==18655==    by 0x401B44: deleteChildrenCallback (server_demo.c:150)
==18655==    by 0x40516A: UA_Server_forEachChildNodeCall (ua_server.c:128)
==18655==    by 0x401B98: deleteChildren_r (server_demo.c:158)
==18655==    by 0x401BC0: deleteChildren (server_demo.c:164)
==18655==    by 0x401F07: main (server_demo.c:201)
==18655==  Address 0x5234920 is 0 bytes inside a block of size 10 free'd
==18655==    at 0x4C2D1CC: free (vg_replace_malloc.c:473)
==18655==    by 0x4022EC: String_deleteMembers (ua_types.c:85)
==18655==    by 0x4028E4: NodeId_deleteMembers (ua_types.c:194)
==18655==    by 0x402CA1: ExpandedNodeId_deleteMembers (ua_types.c:304)
==18655==    by 0x40468F: deleteMembers_noInit (ua_types.c:950)
==18655==    by 0x40472A: UA_deleteMembers (ua_types.c:964)
==18655==    by 0x41140D: UA_ReferenceNode_deleteMembers (ua_types_generated_handling.h:1474)
==18655==    by 0x414D6F: deleteOneWayReference (ua_services_nodemanagement.c:1241)
==18655==    by 0x41E2DF: UA_Server_editNode (ua_server_utils.c:262)
==18655==    by 0x414EEA: Service_DeleteReferences_single (ua_services_nodemanagement.c:1260)
==18655==    by 0x4149FA: removeReferences (ua_services_nodemanagement.c:1159)
==18655==    by 0x414AFE: Service_DeleteNodes_single (ua_services_nodemanagement.c:1183)
==18655==
==18655== Invalid read of size 1
==18655==    at 0x4C32015: __memcmp_sse4_1 (vg_replace_strmem.c:972)
==18655==    by 0x4022BC: UA_String_equal (ua_types.c:78)
==18655==    by 0x402B68: UA_NodeId_equal (ua_types.c:263)
==18655==    by 0x40D5D9: findNode (ua_nodestore.c:107)
==18655==    by 0x40DD10: UA_NodeStore_remove (ua_nodestore.c:306)
==18655==    by 0x414B15: Service_DeleteNodes_single (ua_services_nodemanagement.c:1185)
==18655==    by 0x414C40: UA_Server_deleteNode (ua_services_nodemanagement.c:1216)
==18655==    by 0x401B44: deleteChildrenCallback (server_demo.c:150)
==18655==    by 0x40516A: UA_Server_forEachChildNodeCall (ua_server.c:128)
==18655==    by 0x401B98: deleteChildren_r (server_demo.c:158)
==18655==    by 0x401BC0: deleteChildren (server_demo.c:164)
==18655==    by 0x401F07: main (server_demo.c:201)
==18655==  Address 0x5234921 is 1 bytes inside a block of size 10 free'd
==18655==    at 0x4C2D1CC: free (vg_replace_malloc.c:473)
==18655==    by 0x4022EC: String_deleteMembers (ua_types.c:85)
==18655==    by 0x4028E4: NodeId_deleteMembers (ua_types.c:194)
==18655==    by 0x402CA1: ExpandedNodeId_deleteMembers (ua_types.c:304)
==18655==    by 0x40468F: deleteMembers_noInit (ua_types.c:950)
==18655==    by 0x40472A: UA_deleteMembers (ua_types.c:964)
==18655==    by 0x41140D: UA_ReferenceNode_deleteMembers (ua_types_generated_handling.h:1474)
==18655==    by 0x414D6F: deleteOneWayReference (ua_services_nodemanagement.c:1241)
==18655==    by 0x41E2DF: UA_Server_editNode (ua_server_utils.c:262)
==18655==    by 0x414EEA: Service_DeleteReferences_single (ua_services_nodemanagement.c:1260)
==18655==    by 0x4149FA: removeReferences (ua_services_nodemanagement.c:1159)
==18655==    by 0x414AFE: Service_DeleteNodes_single (ua_services_nodemanagement.c:1183)
==18655==

I've modified my previous server demo to demonstrate this problem when run with a 0.2 git pull from a few hours ago.  It is attached.

[Grant Edwards]

On Thursday, January 19, 2017 at 12:25:29 PM UTC-6, Grant Edwards wrote:

On Thursday, January 19, 2017 at 5:22:29 AM UTC-6, Julius Pfrommer wrote:

Fixed on master and 0.2.

The dangling references are gone, but now I get valgrind errors that I didn't get before this fix.

The errors only seem to happen when I delete a node in a UA_Server_forEachChildNodeCall() callback function.

FWIW, I've found that I can avoid the valgrind errors by not deleting child nodes with UA_Server_forEachChildNodeCall() but using it to build a list of child nodes to be deleted after UA_Server_forEachChildNodeCall() finishes.

But it's sort of ugly, and it's not as general:

#define MaxNodeArray 64

typedef struct nodearray {
  int n;
  UA_NodeId nodes[MaxNodeArray];
} nodearray_t;

static UA_StatusCode listChildrenCallback(UA_NodeId childId, UA_Boolean isInverse, UA_NodeId referenceTypeId, void *handle)

{
  if (isInverse)
    return UA_STATUSCODE_GOOD;
  UA_NodeId organizes = UA_NODEID_NUMERIC(0, UA_NS0ID_ORGANIZES);
  if (UA_NodeId_equal(&referenceTypeId,&organizes))
    {

      nodearray_t *nodearray = handle;
      if (nodearray->n < MaxNodeArray)
        {
          UA_NodeId_copy(&childId,&nodearray->nodes[nodearray->n]);
          nodearray->n++;
        }
    }
  return UA_STATUSCODE_GOOD;
}

static void deleteChildren_r(UA_NodeId *nodeid)
{
  nodearray_t nodes;
  memset(&nodes,0,sizeof nodes);
  UA_Server_forEachChildNodeCall(server,*nodeid,listChildrenCallback,&nodes);

  for (int i=0; i<nodes.n; ++i)
    {
      deleteChildren_r(&nodes.nodes[i]);
      UA_Server_deleteNode(server,nodes.nodes[i],true);
      UA_NodeId_deleteMembers(nodes.nodes+i);
    }
}

[Julius Pfrommer]

Wow. That took some time to figure out.
Fixed via https://github.com/open62541/open62541/commit/a22f371ee33d368f065621652dba95df95974469

This was an unfortunate situation where a reference is deleted from the array that is iterated over.
Imho, this was the only case where this can happen / is exposed to userland.

Best regards,
Julius

[Grant Edwards]

On Thursday, January 19, 2017 at 3:21:49 PM UTC-6, Julius Pfrommer wrote:

Wow. That took some time to figure out.
Fixed via https://github.com/open62541/open62541/commit/a22f371ee33d368f065621652dba95df95974469

This was an unfortunate situation where a reference is deleted from the array that is iterated over.

If you prefer, I think it would be probably be reasonable to prohibit deleting
references from the node whose references are being iterated over.  For
example, in Python you are not allowed to remove items from a list while you
are iterating over that list.

--
Grant