[Prometheus Bug]: PrometheusCOM Interop Issues

RobBryce commented 2 years ago

Contact Details

rbryce@heartlandsoftware.ca

What happened?

A student contacted the group in regards to issues using PrometheusCOM. He is using C# and the COM Interop for access to functionality. The Interop layer was designed for easier access to the COM architecture, from a more modern language than, say, C++.

Investigation shows some odd behaviour. There is is a pair of references creating a 2-way association between fuels with the FBP logic, and the object which contains displayable features. This is set up at fuel creation time. In Prometheus, PrometheusCOM (when used by BurnP3 and Pandora), there is no concern - the 2-way association is constructed correctly. However, when the 2-way association is created from .Net (C#) code, the 2-way association is not created correctly.

We are using a polymorphic datatype (VARIANT) which allows storage of opaque pointers. However, investigation shows that you cannot use an opaque pointer type when putting a value into an object's property. There is other discussion where others have discovered the behaviour I've reported from managed code, and not with native code. .Net raises an MDA error about a potential bad conversion between native and managed types (which makes sense since the code cannot marshal an opaque pointer).

This issue isn't present in PrometheusCOM, but is present in all lowlevel COM objects and the objects they wrap. The solution is change the opaque pointer to another type that can be used to put a value in an object's property. This was quickly verified by changing the pointer type to an 8-byte integer (which is sufficient to hold a pointer value).

I believe this is a change in behaviour for .Net or in the OS since we created the Interop DLL's. This additional security was not discovered when we wrote the scripts to auto-generate these DLL's from the COM DLL's. And there is no problem with Prometheus, BurnP3, or Pandora functionality. If a change is performed, it is minor and would take negligible testing. The change would require a new release of the Prometheus installer since it would impact every wrapper class of a COM object.

Right now, this bug negates the use of the COM Interop DLL's, even though they are part of the installation package.

I do not know if the problem still exists if the COM objects are directly referenced from C#, because Prometheus contains managed code running on the same version of .Net as the test C# code.
I do not know for sure if this is a .Net issue, or an OS issue, so this may impact how Prometheus and related code runs on a future version of the OS. Microsoft's stance on backwards compatibility is slowly evolving.

No change has been made to the source tree for Prometheus and its related DLL's. At this time, this is just investigative, since there's a code freeze on PrometheusCOM (even though the fix is strictly in code that's shared between Prometheus and PrometheusCOM, not unique to PrometheusCOM). However, so long as this is unfixed AND the Interop DLL's are included in the installer, this problem can arise again. This is one example of why I asked if we can:

at minimum, mark PrometheusCOM routines that haven't been validated by BurnP3, as untested in documentation; and/or
return a warning code indicating "use at your own risk"; or
simply remove all unused code from PrometheusCOM - which is possible with a quick analysis of BurnP3 code. (Note that Pandora's implementation hasn't actually been formally validated against the COM.) The motivation is minimize the need for support on untested functionality (or functionality that hasn't been tested in years - like the Interop DLL's).

Without a fix, right now, the student's best options are to:

switch to native C++ like BurnP3 and Pandora; or
move his work to PSaaS.

A conscious decision on the future of the Interop DLL's should be made, since this is the first known project that attempted to use them (outside some testing code), since they were introduced about a decade ago.

Version

(Dev) 2021.12.03

What version of Windows are you seeing the problem on?

Windows 10 64-bit

Relevant log output

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct

spydmobile commented 2 years ago

@RobBryce @BadgerOnABike Do we need more info to action this?

RobBryce commented 2 years ago

The decision on how to act is more political/architectural then technical. The easiest solution is to remove the Interop DLL's from the installer completely.

spydmobile commented 2 years ago

@nealmcloughlin what do you want to do with this? its on hold till you give direction.

BadgerOnABike commented 2 years ago

This seems a little bit like we have an opportunity to use this as some scheduled deprecation. While I understand the desire to allow folks to hit Promtheus with modern languages I wouldn't be against saying "Use C++ or come to PSaaS". Personally I'd prefer to get folks like this into PSaaS as they will be a valuable testing asset over and above what they could do for the visibility of the project.

My vote, leave it annoying, and show them the shiny new racecar.

spydmobile commented 2 years ago

This is an old codebase. We have avoided doing this because it alters the com. we will post a notice rather than modify this old code base. @RobBryce provides a summary, and @spydmobile will alter the website.

spydmobile commented 2 years ago

waiting on the summary from @RobBryce

RobBryce commented 2 years ago

If we remove the Interop DLL's from the installer (and build process), then the ability to use PrometheusCOM.DLL from a managed language like VB or C# is greatly diminished, so I recommend that change. For a summary I would state something like this: "At this time, usage of the PrometheusCOM.DLL with a managed language like C# is no longer supported. There are known issues that are encountered when using these languages. These issues are not slated to be fixed, and the development team has instead decided to focus on development of API's for the PSaaS/WISE project. PrometheusCOM.DLL usage should be limited to C++, and this development team urges new projects engage the WISE project rather than investing into using PrometheusCOM."

nealmcloughlin commented 2 years ago

I support this recommendation.

BadgerOnABike commented 2 years ago

Yea that's a plus 1 from me too. This seems like the cleanest way to move ahead.

WISE-Developers / Project_issues