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

Fix handling of empty SCS17/SCS18 messages #204

Merged

Conversation

rm5248
Copy link

@rm5248 rm5248 commented Nov 8, 2024

The code for handling empty SCS_17 and SCS_18 messages doesn't work correctly. Fix it so that it does.

Because this is non-conforming behavior, added a new flag to optionally enable it.

Copy link
Member

@sidcha sidcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. It would help explaining what is broken (how this patch fixes it) in the commit message; especially for fixes like this. I'm not actively working on LibOSDP all the time and rely heavily on git blame to rebuild context.

@@ -581,4 +581,8 @@ static inline bool is_packet_trace_enabled(struct osdp_pd *pd) {
IS_ENABLED(CONFIG_OSDP_PACKET_TRACE));
}

static inline bool allow_scs17_scs18_empty(struct osdp_pd *pd) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please prefix sc_ to this name to provide some extra context.

Suggested change
static inline bool allow_scs17_scs18_empty(struct osdp_pd *pd) {
static inline bool sc_allow_empty_encrypted_data_block(struct osdp_pd *pd) {

include/osdp.h Outdated
*
* @note this is a PD mode only flag
*/
#define OSDP_FLAG_ALLOW_EMPTY_SCS17_SCS18 0x00200000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this OSDP_ALLOW_EMPTY_ENCRYPTED_DATA_BLOCKS? Users may not know about SCS_17/SCS_18

Suggested change
#define OSDP_FLAG_ALLOW_EMPTY_SCS17_SCS18 0x00200000
#define OSDP_ALLOW_EMPTY_ENCRYPTED_DATA_BLOCKS 0x00200000

include/osdp.h Outdated
@@ -72,6 +72,14 @@ extern "C" {
*/
#define OSDP_FLAG_CAPTURE_PACKETS 0x00100000

/**
* @brief Allow empty SCS_17 and SCS_18 packets. This is non-conforming
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a small write up explaining the "why" here (same could go in the commit message too). This will show up in the documentation site and your wording here should be self contained and convey intent.

src/osdp_phy.c Outdated
Comment on lines 667 to 672
static int warned = 0;
if (!warned) {
LOG_WRN("Received encrypted data block with 0 "
"length; tolerating non-conformance!");
warned = 1;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A LOG_WARN_ONCE() in osdp_common.h macro may come in handy in other places.

Something like,

#define LOG_WRN_ONCE(message, ...) \
    do { \
        static int warned_##__COUNTER__ = 0; \
        if (!warned_##__COUNTER__) { \
            __logger_log(&pd->logger, LOG_WARNING,__FILE__, __LINE__, __VA_ARGS__); \
            warned_##__COUNTER__ = 1; \
        } \
    } while(0)

@rm5248 rm5248 force-pushed the fix-bug-handling-empty-scs17-scs18 branch 2 times, most recently from 23c30f3 to 0c6b65b Compare November 11, 2024 14:05
Add a flag to allow for non-compliant messages for SCS17/SCS18 messages.  Some
OSDP implementations will send an SCS17/SCS18 message with no data when in
secure mode.  This is not correct according to the standard.  If this message
is received with libosdp, the current behavior is to drop the message and
print an error.

This patch adds a new flag, FLAG_ALLOW_EMPTY_ENCRYPTED_DATA_BLOCK to allow for
non-standard behavior of an empty data block with SCS17/SCS18.  This is only
when libosdp is used as a PD.
@rm5248 rm5248 force-pushed the fix-bug-handling-empty-scs17-scs18 branch from 0c6b65b to d6cf02b Compare November 11, 2024 14:07
Copy link
Member

@sidcha sidcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the PR.

@sidcha sidcha merged commit 286bbdb into goToMain:master Nov 11, 2024
12 checks passed
@@ -162,8 +162,7 @@ is how you can run them:
```sh
mkdir build && cd build
cmake ..
make python_install
make check
make check-ut
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change intended?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hunk is not related to this PR (commit message) looked odd to be here; but I'm practicing to avoid nit-picking in reviews :)

But the change itself is sound.

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.

3 participants