Fixed invalid memory read/segfault

[steweg]

This patch fixes segfaults caused by invalid memory read during leafref links removal process. It is tight hash_table implementation, as hash_table removal of record can invalidate all other record pointers due to memory reallocation process

Related to issue #2374

[steweg]

I have not added any specific tests as I was not able to reproduce the issue using just C code. I my project I am using libyang-python, which actually triggers this and I was able to find the issue by using valgrind plugin. I am still not 100% that the proposed patch is correct solution as this behavior of hash_table is not explained in documentation nor expect by the user. I might even suggest to rather have like internal structure of pointers to the final records and if you need to reallocate, reallocate just that part but keep the returned record pointers valid for the caller.
Alternative approach is to avoid using operations with the hash_table in sequence like this:

• get 1st record associate with hash1
• while performing some operation with 1st record, find 2nd record associated with hash2 and remove 2nd record from table
• continue working with 1st record (<-- here you are already working with invalid memory)

Hope it helps to explain the problem. Feel free to propose better solution.

[steweg]

I have rewritten the bugfix based on comment in the issue. I get checks error related to MSVC, but it seems not to be problem of my code, if I understand correctly the output. If there is some issue with my patch just let me know, I will fix it

[michalvasko]

Code changes seem fine, just a minor issue.

But why do you actually need records not to be reallocated? Is that a libyang requirement or of your application and specific usage?

[steweg]

Code changes seem fine, just a minor issue.

But why do you actually need records not to be reallocated? Is that a libyang requirement or of your application and specific usage?

Well as the leafref link records include eventually cyclic reference to each other. On one side leafref is having refrerence its target while target is able to see which leafrefs are pointing to it. The code always build both references and similarly it removes it from references in case of removal. And the removal is the problem as I need to make sure that record entry in memory is still valid regardless of operation done on target node. Unfortunately in case of remove it is not true as removal of reference can lead to having just an empty record with no targets or leafrefs. In that case I wanted to remove the record from good from memory as it is no-use.
So in the end I end-up exactly in scenario that I needed valid pointer to 1st record while I remove 2nd record from hash_table and continue to use 1st records for other operations. Hash table reallocation actually break the 1st record validity.

[michalvasko]

Well as the leafref link records include eventually cyclic reference to each other.

How? I see that struct lyd_leafref_links_rec includes only pointer and arrays of pointers to other nodes, the records themselves are not referenced.

[steweg]

Well as the leafref link records include eventually cyclic reference to each other.

How? I see that struct lyd_leafref_links_rec includes only pointer and arrays of pointers to other nodes, the records themselves are not referenced.

Yes and no. The relationship originally should be as follows:

• node1 1<--->0..n target_nodes
• node1 1<--->0..n leafref_nodes

Because you asked me to not modify lyd_node_term structure, we are using hash_table instead which changes the relationship as follows:

• node1 1<--->0..1 record
• node1 1<--->0..n target_nodes
• record 1<--->0..n leafref_nodes

So the cyclic relationship is there like this:

• node1 --> record1 --> leafref_nodes[x] -> node2 --> record2 --> target_nodes[y] --> node1

Hope it helps to understand the problem

[michalvasko]

So the link node1 -> record1 would be a problem, if it was a direct pointer. But all I found is the function lyd_get_or_create_leafref_links_record(), which finds the record of the node in the hash table. Meaning even if the record is moving in memory after hash table realloc, it will work because there is no direct link (pointer). So I still do not see the issue.

[steweg]

So the link node1 -> record1 would be a problem, if it was a direct pointer. But all I found is the function lyd_get_or_create_leafref_links_record(), which finds the record of the node in the hash table. Meaning even if the record is moving in memory after hash table realloc, it will work because there is no direct link (pointer). So I still do not see the issue.

Yes, it is not directly link/pointer. So let's say that behavior of hash_table is as it is and the scope of my issue just that function. So I have two solutions:

1. Rewrite the code as I did right now to use pointer value as hash_table records
2. Rewrite the code in a way that I will perform additional lookups in hash_table after the possible "reallocation" (this was my initial version of patch)

Either way it will work for my case. I would prefer to keep the current proposed solution (1). Is that ok with you? Or do you prefer different solution?

[michalvasko]

I think there is another solution.

3. Make all the functions returning records return a copy of a record instead of a pointer to it.

And I think I prefer this one except there is one API function lyd_leafref_get_links(). But you can easily write a new one and deprecate this one. We are in the process of preparing libyang v4, so it will soon be properly changed, anyway. What do you think?

[steweg]

I don't think it is good idea. The record itself includes two arrays of pointers to nodes, so if I return a copy, shall this be copied as well (a.k.a. deepcopy) or just copy array pointer (a.k.a. shallow copy). I guess you are assuming doing shallow-copy. But then when it comes to array deletion, the pointer in hash_table record can be left in invalid state after removal of final element from array. Deepcopy is even worse.

The other question would be who would release the memory of the "copy" record and when? I can imagine that it can be put on caller stack, so freeing memory will be done automatically once the record is not needed anymore.

That being said, I would rather not change it in this way. But rather use solution 1 or 2.

[michalvasko]

Okay, fair enough, solution 1 will do.