Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASN1 structure usage and AccessViolationException in MSVC #172

Closed
khevessy opened this issue Oct 21, 2022 · 14 comments
Closed

ASN1 structure usage and AccessViolationException in MSVC #172

khevessy opened this issue Oct 21, 2022 · 14 comments

Comments

@khevessy
Copy link
Contributor

Hi all,

I've managed to compile Vanetza in a C++/CLI environment and want to use it as a DLL library under C#.
I am trying to use ASN.1 structures. It doesn't matter if I use the cpp wrapper classes or not.

It is enough if I use this line:

vanetza::asn1::Cam message;

and program fails with this error

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
Repeat 2 times:
--------------------------------
   at <Module>.vanetza.asn1.free(asn_TYPE_descriptor_s*, Void*)
--------------------------------
   at NativeCodeWrapper..ctor()
   at Example.Main()

I also tried

CAM_t* cam = vanetza::asn1::allocate<CAM_t>();
// Fill in the structure
vanetza::asn1::free(asn_DEF_CAM, cam);

and it fails with the same error. It is apparent that it is caused by trying to free the structure. The address seems correct to me (usually something around 0x00000267B4216AD0).
Does anyone have an idea what could be causing this?

Regards,
Karel

@riebl
Copy link
Owner

riebl commented Oct 23, 2022

Though asn1c is far from perfect, I don't think it's the culprit this time. I have just run extensive memory checks with Valgrind on the GTest_ItsAsn1 unit test target without any findings. Thus, I suppose the issue may be caused by some build configuration for MSVC, e.g. some preprocessor statements testing for MSVC or Win32 and thus adapting the source code. This would at least explain why no memory error is detected on my Linux build.

@khevessy
Copy link
Contributor Author

khevessy commented Oct 24, 2022

I do not know if it would help, but I have some compilation warnings, maybe some of them are relevant?

EDIT: I have removed the wall of text with the build log as I do not believe the warnings were relevant to the discussion.

Build fails the first time but is successful the second time.

@khevessy
Copy link
Contributor Author

khevessy commented Oct 25, 2022

Some other notes:

  • Same exception occurs if the CAM message is created in a module that was built without CLR support
  • Size of the allocated structure is correct (same as in Linux)

@khevessy
Copy link
Contributor Author

khevessy commented Oct 25, 2022

I have tried using of the CAM_t structure directly as per author of the other issue and strange thing is that it does not work either.

When I try to use function asn_encode_to_buffer() in Windows, it returns -1 to the .encoded field of the return value. Furthermore, errno is set to ENOENT - encoding transfer is not defined. When I run the same code on Linux, it encodes the message correctly. See the following code sample for illustration (setting of the CAM fields is shortened for clarity but I am doing it):

CAM_t* message = new CAM_t();

// Setting the CAM fields (header, basic container, high frequency container)
// ...

uint8_t buffer[1024];
asn_enc_rval_t ret = asn_encode_to_buffer(0, ATS_UNALIGNED_BASIC_PER, &asn_DEF_CAM, message, buffer, 1024);
// On Windows, asn_encode_to_buffer returns -1 to ret.encoded
if (-1 == ret.encoded)
{
    std::cout << "Error: " << errno << ", " << erv.failed_type->name << std::endl;
    // Windows - errno is 2 (ENOENT)
}
else
{
    std::cout << "CAM successfully encoded!" << std::endl;
    // On Linux, it goes here. When I check the buffer manually, there are correct CAM data.
}

What may be relevant: description of how I am exporting the symbols from the DLLs: When building, I set CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS CMake variable to true (-DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=TRUE on the command line). This however exports only functions/classes and not data symbols. Those which I need I am exporting manually in the header files. I think the problem may be in the ASN.1 DLL exported symbols. I am exporting them like this (example from vanetza/asn1/its/CAM.h):

#ifdef asn1_its_EXPORTS
__declspec(dllexport) asn_TYPE_descriptor_t asn_DEF_CAM;
#else
__declspec(dllimport) asn_TYPE_descriptor_t asn_DEF_CAM;
#endif

or like this (example from vanetza/asn1/support/constr_SEQUENCE.h):

#if defined (asn1_its_EXPORTS) || defined (asn1_security_EXPORTS) || defined (asn1_pki_EXPORTS) || defined (asn1_support_EXPORTS)
__declspec(dllexport) asn_TYPE_operation_t asn_OP_SEQUENCE;
#else
__declspec(dllimport) asn_TYPE_operation_t asn_OP_SEQUENCE;
#endif

I would have thought that if the symbols were exported/imported incorrectly, project would not pass linking. But it seems strange to me that the compiler would have some problem with the asn_DEF_CAM variable - what could be other reason for returning ENOENT?

@khevessy
Copy link
Contributor Author

khevessy commented Oct 26, 2022

Culprit is that asn_DEF_CAM.op (it should be set to &asn_OP_SEQUENCE) has all the callbacks set to 0. However, asn_OP_SEQUENCE itself has them set correctly. I think that it is happening because one global symbol from one DLL is initialized using another symbol from another DLL (I am just guessing). I think I am using the __declspec(dllimport) and __declspec(dllexport) directives incorrectly. I have posted a relevant question to stackoverflow as it is more a question about Windows than it is about Vanetza.
I have tried assigning

asn_DEF_CAM.op = &asn_OP_SEQUENCE;

in my program but it still fails - there is probably some other unitialized pointer somewhere. Now the asn_encode_to_buffer() function does not return ENOENT but I am back to the AccessViolationException.

@riebl
Copy link
Owner

riebl commented Oct 26, 2022

Phew, I have never entered the dungeons of DLL symbols. If that is really the causing issue, then an AccessViolationException is not exactly pinpointing in that direction. At least not my first guess ;-)

@khevessy
Copy link
Contributor Author

The truth is that when I try to dereference a null pointer, I get different exception: System.NullReferenceException. But when I try to dereference a function pointer that is pointing to 0, I get the System.AccessViolationException.

@riebl
Copy link
Owner

riebl commented Oct 26, 2022

In the end, I would love to have Windows/MSVC as another target included in our GitHub actions executed for every commit. A few years ago I built Vanetza on Windows out of curiosity but not with MSVC. I had no use case for this, though.

@khevessy
Copy link
Contributor Author

khevessy commented Oct 31, 2022

I think that with DLLs, ASN1 structures cannot be initialized the way they are now. Problem is that it is impossible to use address of dllimported symbol in an initializer.
I managed to encode the CAM messages and also use your wrapper class without crash. My solution is to add a C file to each asn1 project (asn1_its, asn1_pki, asn1_security) and in that file, manually include the C files with definition of the needed symbols (constr_SEQUENCE.c, NativeInteger.c and so on). That way, each DLL has its own copy that it uses. But this probably is not something you would want in your codebase.

@khevessy
Copy link
Contributor Author

In the end, I would love to have Windows/MSVC as another target included in our GitHub actions executed for every commit. A few years ago I built Vanetza on Windows out of curiosity but not with MSVC. I had no use case for this, though.

But you built it as a static library, no? I think this could be added to the actions as you are saying. I do not know how to do that though.

@riebl
Copy link
Owner

riebl commented Nov 3, 2022

But you built it as a static library, no? I think this could be added to the actions as you are saying. I do not know how to do that though.

Yes, I think so, but I don't remember the details anymore. Building static libraries should work for C++/CLI as well, shouldn't it?

@khevessy
Copy link
Contributor Author

khevessy commented Nov 4, 2022

Yes, static libraries work fine.

As for the DLLs, there is one more problem - I had to use define CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS so that all the Vanetza functions are exported (windows does not do this automatically, everywhere would have to be the __declspec(dllexport) directive). But there is a known bug that there are exported some standard library symbols that shouldn't be and the linking then fails. One solution is to comment out the offending definitions in the resulting .def export file, other solution is to use the /FORCE directive for the linker so that it ignores multiply defined symbols. This works for me, however it is not the most clean solution.

If you wanted, I can write instructions with all the steps needed to do the build under MSVC as there were few more workarounds I had to do. Dirtiest was adding a dummy member to empty unions in vanetza/asn1/its/RegionalExtension.h, as it seems that standard C does not allow this but it is ignored by gcc (MSVC error C2016: C requires that a struct or union has at least one member).

I think I can close this issue now as everything seems to work now (with the structures put in the .c files as mentioned in my previous comment). It is a fact that address of dllimported symbol cannot be used in a static data initializer. Other solution would be to initialize the structures in a function when the DLL is loaded, but including the definitions seemed simpler to me.

Should I create another issue with adding static library build to the GitHub actions as you mentioned?

@riebl
Copy link
Owner

riebl commented Nov 5, 2022

Regarding the RegionalExtension unions: GCC issues a lot of warnings regarding these empty unions. Maybe we can add a dummy field to each empty union via our asn1c patch mechanism.
I would be more than happy to add your MSVC instructions to the documentation! Also building Windows static libraries through GitHub actions would be great so we don't silently introduce any incompatibilities.

@riebl
Copy link
Owner

riebl commented Mar 12, 2023

For future reference: The dummy field _placeholder has been added to empty RegionalExtension unions. See the regional_extension_unions.patch file for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants