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 Potential Buffer Overflow in convertChipType Function #93

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VasenevEA
Copy link

Issue:

The original implementation of the convertChipType function in main.cpp was flagged by the compiler warning, suggesting the possibility of a buffer overflow due to the use of snprintf writing more characters than the buffer size.

main.cpp:1541:36: error: ‘%s’ directive output may be truncated writing up to 557 bytes into a region of size 5 [-Werror=format-truncation=]
 1541 |  snprintf(buffer, sizeof(buffer), "%s", chip);
      |                                    ^~

Fix:

To address this, the function has been updated to first check the length of the chip string. If the length is greater than 4 (the intended size of the buffer minus the null terminator), the function returns 0. This ensures that snprintf never attempts to write more characters than the buffer can handle.

Changes Made:

Added a check to ensure chip string length does not exceed 4 characters.
Returned 0 if the string length check fails.
The updated code is as follows:

static inline uint32_t convertChipType(const char* chip) {
    if (strlen(chip) > 4) {
        return 0;
    }

    char buffer[5];
    memset(buffer, 0, sizeof(buffer));
    snprintf(buffer, sizeof(buffer), "%s", chip);
    return buffer[0] << 24 | buffer[1] << 16 | buffer[2] << 8 | buffer[3];
}

This change should prevent any potential buffer overflows

#47

@RadxaYuntian
Copy link

#85 might be the intended behavior. The original function never returns 0.

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.

2 participants