mfiutil: Fix unsafe assumptions of snprintf(3) return value in function mfi_autolearn_period
#1405
Conversation
| sz -= tmp - buf; | ||
| if (h != 0) { | ||
| tmp += snprintf(tmp, sz, ", "); | ||
| fmt_len = snprintf(tmp, sz, ", "); | ||
| if (fmt_len < 0 || (size_t)fmt_len >= sz) { |
There was a problem hiding this comment.
or maybe just strlcat()? I don't see a need to use snprintf here...
| @@ -50,10 +50,21 @@ mfi_autolearn_period(uint32_t period, char *buf, size_t sz) | |||
|
|
|||
| tmp = buf; | |||
| 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"); | |||
There was a problem hiding this comment.
The proposed change won't hurt but I don't think snprintf can return a negative number here, no?
There was a problem hiding this comment.
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.
I don't think so either. Just pedantic, since I need to cast 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. |
strlcpy(3) should be used. Then the checking for negative number can be eliminated because it returns I didn't use these functions in my fork because they are not portable. |
|
looks like negative return value from snprintf isn't handled still... |
PR: 281160
From my portable mfiutil project, commit https://sourceforge.net/p/mfiutil/code/ci/8ee7bd9112fab7bdf28188fa595f8a8a59cced48/.