This is an archive of the discontinued LLVM Phabricator instance.

Implement snprintf for MSVC with correct return value.
ClosedPublic

Authored by chaoren on May 26 2015, 6:08 PM.

Diff Detail

Event Timeline

chaoren updated this revision to Diff 26560.May 26 2015, 6:08 PM
chaoren retitled this revision from to Implement snprintf for MSVC with correct return value..
chaoren updated this object.
chaoren edited the test plan for this revision. (Show Details)
chaoren added a reviewer: zturner.
chaoren added a subscriber: Unknown Object (MLST).
zturner edited edge metadata.May 27 2015, 9:02 AM

I hadn't thought to write to /dev/null to get the length. Good idea. But why do you check for !errno? The docs just say that if _vsnprintf fails it returns -1, it doesn't say anything about errno.

https://msdn.microsoft.com/en-us/library/1kt27hek.aspx

If buffer or format is NULL, or if count is less than or equal to zero,
these functions invoke the invalid parameter handler, as described in Parameter
Validation https://msdn.microsoft.com/en-us/library/ksazx244.aspx. If
execution is allowed to continue, these functions return -1 *and set *
*errno to EINVAL*.

Yea, I saw that, but it only says that it sets errno in the case that buffer==nullptr or count==0 (not sure why it says count<=0 since count is an unsigned type). So as far as I can tell the value of errno is undefined in the case that buffer, format, and count are all valid, but count is just not big enough (which is really the case we're trying to handle anyway).

Even in the case the MSDN doc talks about, you either won't return from the function at all (i.e. it will abort) or it will return -1. So I think it's best to remove the errno check. Otherwise lgtm.

So as far as I can tell the value of errno is undefined in the case that
buffer, format, and count are all valid, but count is just not big
enough (which is really the case we're trying to handle anyway).

Yeah, errno should be 0 only in the case that count is not big enough,
which is exactly what I'm checking for. (It's the caller's responsibility
to clear errno before calling snprintf). Otherwise, there's an error, and
it should be returned as is, and the fprintf wouldn't be necessary.

So as far as I can tell the value of errno is undefined in the case that
buffer, format, and count are all valid, but count is just not big
enough (which is really the case we're trying to handle anyway).

FWIW, I think it's not defined in the MSDN docs because it's already
defined in the C standard for all system/library calls.

So as far as I can tell the value of errno is undefined in the case that
buffer, format, and count are all valid, but count is just not big
enough (which is really the case we're trying to handle anyway).

FWIW, I think it's not defined in the MSDN docs because it's already
defined in the C standard for all system/library calls.

Well, the whole reason we have to write this function is because Microsoft's implementation doesn't conform :) So I don't think we should assume they conform for anything else either.

In any case, I just tested it with this code:

int main(int argc, char* argv[])
{
    char buffer[10];

    errno = 0;
    snprintf(buffer, 10, "This is a test of the snprintf function and buffer size");

return 0;

}

When snprintf calls _vsnprintf, _vsnprintf returns -1 as expected, and sets errno to 34. So the check is definitely wrong currently.

Well, the whole reason we have to write this function is because
Microsoft's implementation doesn't conform :) So I don't think we should
assume they conform for anything else either.

https://memegen.googleplex.com/templateimage?template=burn

Should we just check for ERANGE (34) then? We need to save and restore
errno in that case to conform with the C standards. But I'm not sure about
using an undocumented behavior.

amccarth added inline comments.
source/Host/windows/Windows.cpp
228

I think there's still a problem here. _vsnprintf will not terminate if the intended content exactly fills count. My understanding on other platforms is that vsnprintf will always terminate.

I'm reading between these lines from MSDN:
"vsnprintf ,_vsnprintf, and _vsnwprintf return the number of characters written if the number of characters to write is less than or equal to count; if the number of characters to write is greater than count, these functions return -1 indicating that output has been truncated. The return value does not include the terminating null, if one is written."

"If there is room at the end (that is, if the number of characters to write is less than count), the buffer will be null-terminated."

Well, the whole reason we have to write this function is because
Microsoft's implementation doesn't conform :) So I don't think we should
assume they conform for anything else either.

https://memegen.googleplex.com/templateimage?template=burn

Should we just check for ERANGE (34) then? We need to save and restore
errno in that case to conform with the C standards. But I'm not sure about
using an undocumented behavior.

I don't think we should check for ERANGE because the documentation doesn't even say that it returns ERANGE. I think we should just check for -1 and ignore errno. If you want to save and restore errno you can do that by saving it to a temporary variable first, and restoring it if _vsnprintf returns >= 0.

I'm not sure if that will be 100% standards conforming, but it will at least work correctly :)

chaoren updated this revision to Diff 26615.May 27 2015, 11:03 AM
chaoren edited edge metadata.
  • Correct handling of errno for when count <= bytes (would be) written.
zturner accepted this revision.May 27 2015, 12:35 PM
zturner edited edge metadata.

looks good.

This revision is now accepted and ready to land.May 27 2015, 12:35 PM

I still think there's a bug where the string can be unterminated.

I still think there's a bug where the string can be unterminated.

Do you mean the case where the number of characters written (no terminating
'\0') is the same as count? buffer[count-1] = '\0' should handle that.

Oh, I missed that line. Yes, that's what I meant and it looks like this handles it. Thanks.

This revision was automatically updated to reflect the committed changes.