mb2hal: fix core dump on version 1.0

[sensille]

Always check if pin memory is really allocated before using it. When running as version 1.0, some pins are not allocated.

This is one of many possible solutions. I decided against checking the version here and instead always check as I think it is a bit more robust against future changes.

[SebKuzminsky]

This looks reasonable to me.
Is this change in response to a bug or crash you've seen, or is it preemptively out of an abundance of caution?

[sensille]

No precaution. mb2hal flat out crashes soon after communication starts when trying to use in v1.0 compatibility mode.

[hansu]

I think this has nothing to do with the compatibility mode or not.
E.g. mbtx_03_READ_HOLDING_REGISTERS is not touched by that. For that nothing has changed and it works properly in 2.8.

[sensille]

Yes, I could have added the checks only where strictly needed, but for consistency and maintainability I thought it easier to add them everywhere.

[sensille]

Sorry for not being clear enough about this. Should I prepare a stricter version?

[hansu]

Can you say in which case it crashes in your configuration?

[sensille]

[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `mb2hal config=e100.ini'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x000055f95ba03073 in fnct_16_write_multiple_registers (this_mb_tx=0x55f95d46b5e0, this_mb_link=0x55f95d46af40)
at hal/user_comps/mb2hal/mb2hal_modbus.c:274
274 int data32 = (uint16_t) float_val + *(this_mb_tx->int_value[counter]);
[Current thread is 1 (Thread 0x7ff7852eb700 (LWP 22650))]
(gdb) bt
#0 0x000055f95ba03073 in fnct_16_write_multiple_registers (this_mb_tx=0x55f95d46b5e0, this_mb_link=0x55f95d46af40)
at hal/user_comps/mb2hal/mb2hal_modbus.c:274
#1 0x000055f95b9ff27b in link_loop_and_logic (thrd_link_num=) at hal/user_comps/mb2hal/mb2hal.c:212
#2 0x00007ff785a86fa3 in start_thread (arg=) at pthread_create.c:486
#3 0x00007ff78588b06f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

[hansu]

Yes, I could have added the checks only where strictly needed, but for consistency and maintainability I thought it easier to add them everywhere.

You added these checks only for register-based modbus transactions. Wouldn't it be also good to have them also for coil-based ones?

[sensille]

You're totally right. For some reason I just focussed on float_value/int_value. It is also necessary at least for bit_inv.

I prepared a commit: 1f4ea53

I again added the check to both bit and bit_inv, although it isn't strictly necesasry for bit. Unfortunately I can't easily test it, because my spindle doesn't support that access.

[hansu]

Thanks.
To cover everything, this check also needs to be added to fnct_05_write_single_coil and fnct_15_write_multiple_coils.
Even though it would be more important to find the root why these checks are necessary for some functions.
It seems that my implementation of the backward-compatibility version is missing something.
Unfortunately I haven't any hardware here right now to test it detailed.

[sensille]

Oh, the root cause is simple. The memory for the float/int/.../value arrays get allocated in mb2hal_hal.c, and zeroed. Those arrays contain the pointers to the hal pins and get set with hal_pin_xxx_newf. But on version 1.0, some of those allocs get skipped, because the pins are not created, leaving the array elements as NULL. But mb2hal_modbus.c used to write to/read from those pointers regardless of the version, thus dereferencing NULL.
The strictest way to fix it would have been to also add version checks (or null pointer checks) only to those pins that are not present in v1.0. I just thought it to be less error prone to add it to all accesses, as someone extending the code at a later point it time might not be aware of the dependency. I did not add the check to 05 and 15_ as there is only ever one pin to read and without that pin sending a default value would not make much sense.

[hansu]

Yeah I see it I should have made the distinction of version also in mb2hal_modbus.c where you added the NULL pointer checks.
Your check is more universal than checking the version number, but it's not self-explaining why it's needed. Maybe we should add a comment.

I think fnct_01_read_coils will be fine but fnct_02_read_discrete_inputs will crash, so it will be good to get your proposed fix in.

[sensille]

Given the argument of self-explaning, I made a patch with version checks instead:

95e7235 (based on a revert of the previous fix).

I think it is fine, too. You decide :)

[hansu]

Given the argument of self-explaning, I made a patch with version checks instead:

95e7235 (based on a revert of the previous fix).

I like that version, thanks :)
Can you prepare a pull request to let us see if the others share my opinion?

[sensille]

To be honest, I like this version better, too. It turned out much smaller than I expected. I'll prepare the PR tomorrow.

[sensille]

#2298