Skip to content

mfiutil: Fix unsafe assumptions of snprintf(3) return value in function mfi_autolearn_period #1405

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.

Sign up for GitHub

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

Jump to bottom
Closed
Low-power wants to merge 1 commit into from
Closed

mfiutil: Fix unsafe assumptions of snprintf(3) return value in function mfi_autolearn_period #1405

Low-power wants to merge 1 commit into from

Conversation

@Low-power
Copy link
Contributor

@Low-power Low-power commented Sep 3, 2024 •
edited
Loading

delphij
delphij reviewed Sep 6, 2024
View reviewed changes
usr.sbin/mfiutil/mfi_bbu.c
if (h != 0) {
tmp += snprintf(tmp, sz, ", ");
fmt_len = snprintf(tmp, sz, ", ");
if (fmt_len < 0 || (size_t)fmt_len >= sz) {
Copy link
Member

@delphij delphij Sep 6, 2024

Choose a reason for hiding this comment

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

or maybe just strlcat()? I don't see a need to use snprintf here...

Copy link
Member

@bsdimp bsdimp Sep 21, 2024

Choose a reason for hiding this comment

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

agree'd

delphij
delphij reviewed Sep 6, 2024
View reviewed changes
usr.sbin/mfiutil/mfi_bbu.c
if (d != 0) {
tmp += snprintf(buf, sz, "%u day%s", d, d == 1 ? "" : "s");
int fmt_len;
fmt_len = snprintf(buf, sz, "%u day%s", d, d == 1 ? "" : "s");
Copy link
Member

@delphij delphij Sep 6, 2024

Choose a reason for hiding this comment

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

The proposed change won't hurt but I don't think snprintf can return a negative number here, no?

Copy link
Member

@bsdimp bsdimp Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It returns the number of characters that would be written if there was an unlimited buffer. It might be > sz.
However, it can return < 0 if there's an error in formatting (well, the man page just says "an error") so we return at least an empty buffer.

@Low-power
Copy link
Contributor Author

Low-power commented Sep 6, 2024

The proposed change won't hurt but I don't think snprintf can return a negative number here, no?

I don't think so either. Just pedantic, since I need to cast fmt_len into size_t to compare it with sz; better to make sure it isn't negative first.

On other hand, the man page did mentioned a few error conditions for this function. It is theoretically possible for snprintf(3) to fail due to an internal buffer allocation failure in the C library.

@Low-power
Copy link
Contributor Author

Low-power commented Sep 6, 2024 •
edited
Loading

or maybe just strlcat()? I don't see a need to use snprintf here...

strlcpy(3) should be used. Then the checking for negative number can be eliminated because it returns size_t.

I didn't use these functions in my fork because they are not portable.

@bsdimp bsdimp self-assigned this Oct 4, 2024
@bsdimp bsdimp added the changes-required Cannot land as is, change requested of submitter label Oct 4, 2024
@bsdimp
Copy link
Member

bsdimp commented Oct 4, 2024

looks like negative return value from snprintf isn't handled still...

@bsdimp bsdimp added the merged Closed commit that's been merged label Jun 13, 2025
@bsdimp
Copy link
Member

bsdimp commented Jun 13, 2025

Automated message from ghpr: Thank you for your submission. This PR has been merged to FreeBSD's branch. These changes will appear shortly on our GitHub mirror.

@bsdimp bsdimp closed this Jun 13, 2025
freebsd-git pushed a commit that referenced this pull request Jun 13, 2025
@Low-power @bsdimp
mfiutil: Fix unsafe assumptions of snprintf(3) return value
aae67a2 
PR: 281160
Reviewed by: imp
Pull Request: #1405
Closes: #1405
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bsdimp bsdimp bsdimp left review comments

+1 more reviewer

@delphij delphij delphij left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@bsdimp bsdimp

Labels

changes-required Cannot land as is, change requested of submitter merged Closed commit that's been merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Low-power @bsdimp @delphij