logmanoriginal / lvssh2

LabVIEW­ bindings for libssh2
BSD 3-Clause "New" or "Revised" License
0 stars 3 forks source link

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

Closed j-medland closed 1 month ago

j-medland commented 6 months ago

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 commented 6 months ago

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:

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.