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

Fixed byte-packing and moved to NumericResizeArray for LStrHandle allocation #2

Conversation

j-medland
Copy link
Contributor

I had a review of your lvssh-extension C/C++ code and noticed that you were always applying the byte-packing compiler directive regardless of the bitness.

Only 32-bit builds need to have this applied, 64-bit builds just use the normal byte-packing. Luckily NI provide two headers lv_prolog.h and lv_epilog.h which automatically handle this.

I have also modified how you are allocating memory for LabVIEW strings.

As an LV-String has the form

LStr {
   int32_t cnt;
   uChar * str
}

it is also affected by the byte-packing, so allocating the correct number of bytes depends on system bitness. Luckily LV-strings have the same structure as 1-D lv-arrays so we can use NumericArrayResize to allocate a U8 array (i.e chars) which will automatically account for any padding between the count and the string-buffer pointer.

Finally - I changed how a char data-buffer is wrriten to an LStrHandle. By using a memcpy/MoveBlock, we can ensure that we are only copying in the number of bytes specified as the data-buffer length instead of relying on a null-termination character in the data-buffer.

The code builds successfully but I am not sure which examples or tests are suitable to ensure that these modifications are fully tested. I have not included the modified built-dlls from this code as I imagine that you would want to build these yourself.

@logmanoriginal
Copy link
Owner

This is very insightful, thanks for looking into it!

Only 32-bit builds need to have this applied, 64-bit builds just use the normal byte-packing. Luckily NI provide two headers lv_prolog.h and lv_epilog.h which automatically handle this.

This explains why some of the types did not properly map to their corresponding LabVIEW types 🤯

The code builds successfully but I am not sure which examples or tests are suitable to ensure that these modifications are fully tested.

The affected examples are:

  • debug_trace.vi
  • session_callback.vi
  • userauth_keyboard_interactive.vi
  • userauth_publickey.vi

I made a few tests and found that userauth_keyboard_interactive.vi was crashing with access violation due to the changed byte-packing when called in LabVIEW 64-bit. This is easily fixed by changing lvssh2_userauth_keyboard_interactive_response_function_input_args_64.ctl so that prompt is a 64-bit numeric instead of 32-bit.

Both userauth_keyboard_interactive.vi and userauth_publickey.vi now still crash with a heap corruption exception. I'm not quite sure why, though.

@logmanoriginal logmanoriginal force-pushed the feature-labview-memory-manager-fix branch from 3e945c5 to 718998a Compare September 28, 2024 08:58
@logmanoriginal logmanoriginal merged commit 718998a into logmanoriginal:main Sep 28, 2024
1 check failed
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

Successfully merging this pull request may close these issues.

2 participants