goToMain / libosdp

Implementation of IEC 60839-11-5 OSDP (Open Supervised Device Protocol); provides a C library with support for C++, Rust and Python3
https://libosdp.sidcha.dev
Apache License 2.0
130 stars 69 forks source link

Failed to decode reply KEYPPAD(53) when osdp_KEYPAD response is correct #86

Closed etorresmn closed 1 year ago

etorresmn commented 1 year ago

I made a very basic application just to run this library but every time I press a key from the keypad of my reader I get a "Failed to decode reply KEYPPAD(53)" error. Looking at the data trace I can see the packet from the PD is correct. Here is a screenshot

keyppad_fail2

The packet FF FF 53 81 0B 00 06 53 00 01 34 95 62 (osdp_KEYPAD) is correct however the library is not able to parse it correctly. Here is the complete log of the output of this basic application, it has some messages and one command I am sending from my little application keypad_fail.log

As it didn't matter what key I pressed or when I dug a little bit in the code and added some log messages in the driver to know what was going wrong, here is the output of the error.

keyppad_fail

The yellow arrow is a log message in the first line of the REPLY_KEYPPAD case in cp_decode_response function, you can see that the length len variable value is 4 which is already incorrect, the green arrow points a hexdump of the buffer that should contain the packet called immediately after the log message marked by the "KEYPPAD event detected" message. You can see the buffer is incorrect as well, the first byte is the command id, not the SOM and here I think is where the problem is, the function that parse the message is probably confusing the command ID with the SOM and hence splitting the packet. The last blue arrow shows a log message placed after detecting the press of a key, at this point you can see the data length is completely incorrect, 195 is the value of the first byte of the CRC after this the length check does not pass and finally I get the error. I leave the log of this last image here.

keypad_fail_custom_msg.log

ivobelitz commented 1 year ago

I ran into the same issue. The same issue also occurs when the reply is of type REPLY_RAW. The issue seems to be that the code first skips over the data fields of the reply message by incrementing pos, but then tries to store the values of the same data fields. The fix would be to set the event parameters as you're going through the reply message. An example fix:

case REPLY_KEYPPAD:
    if (len < REPLY_KEYPPAD_DATA_LEN) {
        break;
    }
    event.keypress.reader_no = buf[pos++]; /* reader number */
    event.keypress.length = buf[pos++]; /* key length */
    if ((len - REPLY_KEYPPAD_DATA_LEN) != event.keypress.length || !ctx->event_callback) {
        break;
    }
    event.type = OSDP_EVENT_KEYPRESS;
    for (i = 0; i < event.keypress.length; i++) {
        event.keypress.data[i] = buf[pos + i];
    }
    ctx->event_callback(ctx->event_callback_arg, pd->idx, &event);
    ret = OSDP_CP_ERR_NONE;
    break;
etorresmn commented 1 year ago

@ivobelitz thank you very much. The fix works =)

sidcha commented 1 year ago

@etorresmn, thanks for the detailed bug report and @etorresmn for the fix. It was a bug introduced during a rebase and there was a gap in testing so I never caught it. Will publish fix and add tests now.