fix: Fixed UB dereference because of cast (EPROT-64)

[bartman101]

Description

event_info->opt_fd is of type int16_t however it is referenced and then casted to an int*. The dereference of that int* in mb_drv_get_next_node_from_set could (and did) cause undefined behavior.

When an error has occured, the mbm_on_error is called which calls mb_drv_check_node_state with a reference of event_info->opt_fd, which is a int16_t and gets casted to int*. It internally calls mb_drv_get_next_node_from_set where the fd_ptr is dereferenced and four bytes are being copied into fd. See the outcome below:

D (34446) mb_driver: 0x3ffe33c4, fd event get: 0x40:0, |MB_EVENT_ERROR
D (34453) mb_driver: fd_ptr (*fd_ptr): [-720896], (*(int16_t*)fd_ptr) [0]
D (34459) mb_driver: fd:[-720896]
D (34462) mb_driver: fd:[-720895]
D (34465) mb_driver: fd:[-720894]
Guru Meditation Error: Core  1 panic'ed (LoadProhibited). Exception was unhandled.

Within mb_driver I have added a log as test which looks like the following:

ESP_LOGD(TAG, "fd_ptr (*fd_ptr): [%d], (*(int16_t*)fd_ptr) [%d]", *fd_ptr, *(int16_t*)fd_ptr);

Currently trying to merge it into bugfix/fix_slave_error_disconnected_node because this can also happen within port_tcp_slave but has been fixed there in #162. If you want me to instead merge into main, please let me know.

Related

• Bugfix/fix slave does not gracefully close sockets #162

Testing

It happens for me when a master is connected to a client and the client resets. The log above is shown and the controller crashes with a LoadProhibited. This doesn't happen anymore with the fix. I'm not really sure why we get the MB_EVENT_ERROR but that doesn't really matter.

Might be hard to reproduce because of undefined behavior doing strange things but can be seen as working in the same log but not crashing below:

D (22844) mb_driver: 0x3ffe5cf8, fd event get: 0x40:0, |MB_EVENT_ERROR
D (22850) mb_driver: fd_ptr (*fd_ptr): [0], (*(int16_t*)fd_ptr) [0]
D (22855) mb_driver: fd:[0]
D (22858) mb_driver: 0x3ffe5cf8, node: 0, sock: 55, IP:192.168.6.2, check connection timeout = 21968, rcv_time: 11416 2000

---

Checklist

Before submitting a Pull Request, please ensure the following:

• 🚨 This PR does not introduce breaking changes.
• All CI checks (GH Actions) pass.
• Documentation is updated as needed.
• Tests are updated or added as necessary.
• Code is well-commented, especially in complex areas.
• Git history is clean — commits are squashed to the minimum necessary.

[CLAassistant]

All committers have signed the CLA.

[bartman101]

It now appeared to me that mb_drv_get_next_node_from_set actually changes the value of fd_ptr.

*fd_ptr = fd;

That value will now be lost in my commit due to int curr_fd being local and not set back to event_info->opt_fd. Is this a problem currently? curr_fd could be set back to event_info->opt_fd after the call to mb_drv_check_node_state with a check on max size and an error if out of bounds. What do you think should happen here?

This problem also exists in your bugfix/fix_slave_error_disconnect_node branch in port_tcp_slave.c.

[alisitsyn]

@bartman101 ,

This problem also exists in your bugfix/fix_slave_error_disconnect_node branch in port_tcp_slave.c.

Thank you for your contribution!

Unfortunately, I missed this during my update initially. I already found this aspect as well in my MR and it is still following the formal process and needs to be fixed. The mb_drv_check_node_state has this *fd_ptr as out param and changes it means your code should be changed. Also, this aspect applies to master and slave and needs to be addressed properly. Would you mind if I address this in MR and do not merge your PR?

[bartman101]

@alisitsyn Im fine with not merging this PR and it being fixed properly. Do you mean that you will adres this in your merge request on bugfix/fix_slave_error_disconnect_node? And if so, what time frame would I be looking at? Just checking if its worth it for me to wait for that fix instead of just using this for now on my local fork.

[alisitsyn]

@bartman101 ,

adres this in your merge request on bugfix/fix_slave_error_disconnect_node? And if so, what time frame would I be looking at?

Yes. This complete fix will be officially represented in v2.1.3 in June. Then It is better for you to use your fork for now. I will update my branch ASAP.