Recommended usage of SoFullPath exhibits undefined behavior

[dodomorandi]

This is a quote from SoFullPath.cpp doc:

Since the SoFullPath is derived from SoPath and contains no private data, you can cast SoPath instances to the SoFullPath type.

This suggestion is wrong because, as also explained here, you cannot instantiate a base class and cast it to a derived type, even if there are no additional fields in the derived class.

The issue is caught by the undefined behavior sanitizer using GCC 12.2.1 (probably even with older version) with the following snippet:

#include <Inventor/SoFullPath.h>

#include <cstdio>

int main() {
    auto path = static_cast<SoFullPath*>(new SoPath(10));
    std::printf("%d", path->getLength());

    // Nope, ~SoPath() is protected. Must leak...
    // delete static_cast<SoPath*>(path);
}

The code, assuming the snippet is called so_full_path_casting.cpp, can be compiled with:

g++ -Wall -Wextra -Wpedantic -fsanitize=undefined so_full_path_casting.cpp -o so_full_path_casting -lCoin 

It is also impossible to correctly destruct and deallocate the instance, as noted in the comment, but that is another can of worms (the issue is even more clear if you try to use a std::unique_ptr instead of the new, because it won't compile).

---

I generally like to be constructive when raising an issue, proposing possible solutions or at least some ideas. In this case I have to say sorry because I don't see a good approach: other projects depends on this exact behavior (because it is the recommended way), therefore changing the architecture of SoPath and SoFullPath could easily lead to subtle breaking changes (and exhibit effects of UB, which seem to be silent in this case). From my perspective, the simplest approach is extremely effective from a technical point of view: making the constructor of SoFullPath public in order to make things work in the most intuitive way. However, it's hard to perceive this kind of changes from other projects, it will probably go unnoticed. On the other hand, if you explicitly opt for a noticeable breaking change (like, for instance, totally removing SoFullPath), people will be probably angry.

As already said, I don't see a good approach, I hope you are better than me at finding one. In any case, thank you for your efforts.

[Renreok]

Can you please add the error message you get from gcc when compiling your code? Unfortunately I don't have a gcc environment set up for building Coin, so I tried to compile the code from the refereced Stack Overflow discussion on onlinegdb.com (which seems to use g++ 11.3.0), but it runs without any complaints.

I'm not an expert on the C++ standard, so I cannot tell whether this code is valid or causes undefined behavior. However, the discussion in the referenced Stack Overflow discussion does not clearly indicate to me that the code is not valid. The referenced paragraph from the C++ standard even seems to state that in some situations such a conversion is allowed. But, as said, my knowledge about C++ standard terminology is very limited (e.g. what is meant by a "base class subobject"? multiple inheritance?). Since we have a very special situation (e.g. no virtual inheritance, no multiple inheritance, no data members in the derived class), it's hard to tell whether this is undefined behavior without real deep knowledge of the whole C++ standard.

What I can tell is that this problem is not limited to the Coin implementation of Open Inventor. This design goes back to the Open Inventor 2.1 API (and probably even beyond) and is even described in the Inventor Mentor books. It is applied in the whole Open Inventor world and it seemed to be working with all major compilers for more than 20 years (what does not mean that it might not break with the next compiler patch release...). However, I've never seen something similar in another library.

I guess we all agree that one should not use such a design in the grey areas of C++, but as you said - this will be very hard to change. Of course we could think about adding a more safe API (e.g. by creating SoFullPath as a wrapper around SoPath), but at the moment I would hesitate to do so for the reasons above.

Regarding your question about the destruction of an SoPath object: As with every other SoBase derived object, you can delete it by calling ref() and unref(). This is also a design, one would not apply anymore today, but it's so fundamental to Open Inventor, that it is impossible to change.

[dodomorandi]

Thank you @Renreok for your reply!

Can you please add the error message you get from gcc when compiling your code? Unfortunately I don't have a gcc environment set up for building Coin, so I tried to compile the code from the refereced Stack Overflow discussion on onlinegdb.com (which seems to use g++ 11.3.0), but it runs without any complaints.

Sorry if I have been not clear enough. The undefined behavior sanitizier, like other sanitizers, introduces runtime checks in order to detect whether any (detectable) undefined behavior is triggered by the program. Therefore, it's perfectly normal you don't see any issue during compilation, it's something you detect at runtime.

In any case, this is the output you get from the sanitizer:

so_full_path_casting.cpp:6:17: runtime error: downcast of address 0x5617e5165df0 which does not point to an object of type 'SoFullPath'
0x5617e5165df0: note: object is of type 'SoPath'
 00 00 00 00  f8 db 4c a1 79 7f 00 00  00 00 00 d0 00 00 00 00  c0 b9 4f a1 79 7f 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'SoPath'
so_full_path_casting.cpp:7:38: runtime error: member call on address 0x5617e5165df0 which does not point to an object of type 'SoFullPath'
0x5617e5165df0: note: object is of type 'SoPath'
 00 00 00 00  f8 db 4c a1 79 7f 00 00  00 00 00 d0 00 00 00 00  c0 b9 4f a1 79 7f 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'SoPath'

---

I'm not an expert on the C++ standard, so I cannot tell whether this code is valid or causes undefined behavior. However, the discussion in the referenced Stack Overflow discussion does not clearly indicate to me that the code is not valid. The referenced paragraph from the C++ standard even seems to state that in some situations such a conversion is allowed. But, as said, my knowledge about C++ standard terminology is very limited (e.g. what is meant by a "base class subobject"? multiple inheritance?). Since we have a very special situation (e.g. no virtual inheritance, no multiple inheritance, no data members in the derived class), it's hard to tell whether this is undefined behavior without real deep knowledge of the whole C++ standard.

This is the reason I showed the steps to reproduce the problem with the ubsan: we don't need to discuss the nitty-gritty details of the C++ standard, there's already a tool that can detect some of them. Not all of them, unfortunately, but surely in this case we are lucky because we don't have a false negative.

---

What I can tell is that this problem is not limited to the Coin implementation of Open Inventor. This design goes back to the Open Inventor 2.1 API (and probably even beyond) and is even described in the Inventor Mentor books. It is applied in the whole Open Inventor world and it seemed to be working with all major compilers for more than 20 years (what does not mean that it might not break with the next compiler patch release...).

Unfortunately I am aware of this. Indeed, I only realized after a bit that the Open Inventor API I was looking at was from Coin and not from the original code I checked. And, to be honest, there the documentation is even worse because if explicitly says that "it is always safe" 😕. Coin implemented the same API, probably people read that documentation and thought they were doing things right, it's hard to blame them.

But this only makes things worse in a certain way: now Coin should pay the cost of the issue for a design it was not involved into just because it is a maintained project. We can probably agree that even documenting these parts in caps with "don't use this, it's fundamentally broken" and nothing else would just show the project in a bad light, even if it's not the maintainers' fault.

---

I guess we all agree that one should not use such a design in the grey areas of C++, but as you said - this will be very hard to change. Of course we could think about adding a more safe API (e.g. by creating SoFullPath as a wrapper around SoPath), but at the moment I would hesitate to do so for the reasons above.

That's true. Maybe the best thing for the moment to deprecate the usage of these? Let's make a hypothesis: if basically no effort is put into fixing the issue and declaring the interface as deprecated, maybe the same people could invest their time into refactoring the projects currently using these features. Maybe it could be more rewarding and in the end it could improve the overall solution... But that's just an hypothesis.

---

Regarding your question about the destruction of an SoPath object: As with every other SoBase derived object, you can delete it by calling ref() and unref(). This is also a design, one would not apply anymore today, but it's so fundamental to Open Inventor, that it is impossible to change.

Ok, thanks for the info, these are probably the kind of things that should be seen from the perspective of another era of software development ☺️

---

In the end, let me know if you are eventually able to reproduce the bug. In theory it should also be possible to create a minimum testing case of SoPath and SoFullPath using virtual constexpr available in C++20, at that point performing the static_cast would trigger a compilation error if the operation is detected as UB (because the compiler won't make you trigger UB in constexpr context)... but I don't think it's really worth the time 😅. Obviously, if you came up with an idea on how to resolve the problem just let me know 😉.

[Renreok]

The problem can be reproduced with the sample from the linked Stack Overflow discussion when adding a virtual function to the base class with almost every newer version of g++ (I used 9.4.0 and 12.1.0). Without the virtual funtion, the necessary runtime type information is probably missing, or the sanitizer intentionally ignores that case, because in the good old C world (and therefore in the early C++ world) such strange type casts were quite common and in many cases even desired for performance reasons.

Anyway, we have virtual functions in our case, and I think there are little guarantees about the memory layout of objects in the C++ standard in this case. Thus the code is not standard-complient, though it works reliably with at least most major compiler implementations.

What could we do about it? I would see two approaches:

Approach 1: We change the Coin implementation to only instantiate SoFullPath, at least when the instance is accessible from the API layer. The most important instance (SoAction::currentpath) actually is already derived from SoFullPath.

Advantages:

• Solves the problem for the most common cases also for client projects without the need for code change.

Disadvantages:

• Solution cannot be applied to client code, because SoFullPath has a private constructor. However, I think it's rather unusual that clients create SoPath instances directly (instead of copying an existing path using SoPath::copy).
• The same problem exists for SoNodeKitPath, but we cannot create an instance which is both, SoFullPath and SoNodeKitPath, without changing the class inheritance hierarchy.
• Probably some new friend classes need to be added, to enable at least a Coin internal access to the SoFullPath constructor.
• In general, code changes in this area a kind of risky, because one can easily miss something and break existing client code.

Approach 2: We introduce a new class ("SoFullPathAccessor") to access the full path. Simular to SoFullPath, it would be a friend class of SoPath, but instead of accessing SoPath through a casted "this" pointer, it would reference the SoPath instance using a member variable. Usage would be something like

SoFullPathAccessor fullPath(path);
fullPath.getTail();

Advantages:

• Can be used for SoFullPath and SoNodeKitPath.
• No impact on existing Coin code and behavior of client projects.
• Can also be used for SoPath instances created by clients.

Disadvantages:

• Does not solve the problem for existing client projects.
• Introduces additional Coin specific API classes which are not compatible to other Open Inventor implementations.
• If SoFullPathAccessor calls ref() and unref() on the SoPath instance, the SoPath instance might get deleted unexpectedly. Not calling ref() and unref() could lead to a dangling pointer.

Both solutions could also be combined, leading to a solution that combines the advantages of both, but also some of the disadvantages.

[dodomorandi]

Very good analysis indeed, thank you so much ❤️. If you are interested, I created just a very minimal (non) working example using inheritance in constexpr context available in C++20, you can find it here (don't know if it could be useful, but I already tried locally because I was curious). It is interesting to observe that Clang is able to detect the UB at compile time, but GCC is not.

In any case, regarding to the two proposed approach: I don't think I have an unbiased perspective, and surely I don't have a sufficient knowledge of the project in order to being able to correctly choose a right way. With these premises, I think that keep exposing the actual standard API with additional custom layer (in practice your second approach) and some documentation (I'm undecided about eventual deprecation notices, people tend to dislike warnings even when they have a good reason to come up) should be the best approach. And obviously a minor bump with the notes about the issue would help maintainers of other projects.

In the end you don't have the power to magically fix the standard API without breaking the compatibility, therefore creating a non-standard safe layer is the best you can do as Coin project. Hopefully other projects depending on your implementation will be wise and prefer the non-standard API over the broken one, but in the end it's a decision they are free to make.