Skip to content

Correct the behavior and return value of snprintf() #192

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

Merged
merged 1 commit into from
Apr 27, 2025

Conversation

DrXiao
Copy link
Collaborator

@DrXiao DrXiao commented Apr 26, 2025

Recently, I tested the following C code in the test suite:

int main() {
    char buffer[20];
    int written = snprintf(buffer, 0, "Number: %d", -37);
    printf("%s", buffer);
    return written;
}

If using the current shecc to test, the exit code is 0. However, the exit code is 11 if using GCC to validate it.

Due to this difference, I read C99 7.19.6.5 to understand the specification of snprintf() and noticed the following description in that section:

  • If n is zero, nothing is written.
  • Output characters beyond the n-1st are discarded rather than being written to the array, and a null character is written at the end of the characters actually written into the array.
  • The snprintf function returns the number of characters that would have been written
    had n been sufficiently large.

After observing the current implementation of snprintf() in the built-in C library and ran the related test cases again, I found that the behavior and return value of snprintf() did not match the above description.

Therefore, the proposed changes improve the implementation of snprintf() to comply with the specification, and the related test cases are also adjusted to verify the correct behavior.

Summary by Bito

This pull request enhances the snprintf function in the C library to comply with the C99 standard, particularly for zero size parameters. Modifications ensure correct return values and behavior, along with the addition of new test cases for validation, improving reliability across compilers.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 2 - The changes are straightforward and well-documented, making the review process relatively easy.

tests/driver.sh Outdated
Comment on lines 1651 to 1652
# snprintf(). Notice that you should refer to the specification,
# such as C99 7.19.6.5, to understand the behavior of snprintf().
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of saying "you should refer to," quote the essential contents from C standards if you want to emphasize.

@DrXiao
Copy link
Collaborator Author

DrXiao commented Apr 26, 2025

Consider the following code:

int main()
{
    char buffer[20];
    for (int i = 0; i < 19; i++)
        buffer[i] = '0';
    buffer[19] = 0;

    int written = snprintf(buffer, 10, "Number: %06d", -35337);

    for (int i = 0; i < 20; i++)
        printf(" %x", buffer[i]);
    printf("\n");
    return written;
}

When using shecc with the proposed changes and GCC to test, it writes at most 10 bytes.

 4e 75 6d 62 65 72 3a 20 2d 0 30 30 30 30 30 30 30 30 30 0

However, using the original shecc to compile and execute, it writes more than 10 bytes.

 4e 75 6d 62 65 72 3a 20 2d 0 35 33 33 37 30 30 30 30 30 0

This test code also shows that the original implementation of snprintf() is faulty. I will add more test cases to validate and refine the comments in the test suite.

typedef struct {
char *buf;
int n;
} fmtbuf_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe store the number of written bytes or the original buf that we can simply return the string length by them instead of doing the calculation stuff of len?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have defined a new member called len in the structure to store the return value for the functions in the printf() family.

@DrXiao DrXiao force-pushed the libc/fix-snprintf branch from bff2f0b to 65c3e12 Compare April 27, 2025 08:18
lib/c.c Outdated
Comment on lines 213 to 225
/*
* The specification of snprintf() is defined in C99 7.19.6.5,
* and its behavior and return value should comply with the
* following description:
*
* - If n is zero, nothing is written, and s may be a null pointer.
* - Output characters beyond the n-1st are discarded rather than
* being written to the array.
* - The snprintf function returns the number of characters that would
* have been written had n been sufficiently large, not counting
* the terminating null character, or a negative value if an encoding
* error occurred.
*
* Therefore, the following code defines a structure called fmtbuf_t
* to implement formatted output conversion for the functions in the
* printf() family.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shorten the quotation, making the comments lean and still useful.

According to C99 7.19.6.5, it explicitly defines the behavior and
return value of snprintf(). However, the implementation should
consider the following description:

 - If n is zero, nothing is written.
 - Writes at most n bytes, including the null character.
 - On success, the return value should be the length of the
   entire converted string even if n is insufficient to store it.

While validating snprintf() in the built-in C library, it was found
that the behavior and/or the return value did not match the above
description.

Therefore, these changes fix the implementation of snprintf() to
comply with the specification, and the related test cases are also
adjusted to verify the behavior and return value of snprintf().
@DrXiao DrXiao force-pushed the libc/fix-snprintf branch from 65c3e12 to 07d30af Compare April 27, 2025 09:41
Copy link
Collaborator

@vacantron vacantron left a comment

Choose a reason for hiding this comment

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

LGTM

@jserv jserv merged commit 6ae64af into sysprog21:master Apr 27, 2025
6 checks passed
@jserv
Copy link
Collaborator

jserv commented Apr 27, 2025

Thank @DrXiao for contributing!

@DrXiao DrXiao deleted the libc/fix-snprintf branch April 27, 2025 12:03
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.

4 participants