Regarding vsnprintf (Interview)
During the interview, I was asked (among other things) to implement the following function:
int StrPrintF(char **psz, const char *szFmt, ...);
similar to sprintf
, but instead of already allocated storage, the function should itself allocate it and return to a variable *psz
. In addition, it *psz
can point to an already allocated string (on the heap) that can potentially be used during formatting. Naturally, this string should be free by appropriate means.
The return value must be the length of the newly created string, or negative by mistake.
This is my implementation:
int StrPrintF(char **psz, const char *szFmt, ...)
{
va_list args;
int nLen;
va_start(args, szFmt);
if ((nLen = vsnprintf(NULL, 0, szFmt, args)) >= 0)
{
char *szRes = (char*) malloc(nLen + 1);
if (szRes)
if (vsnprintf(szRes, nLen + 1, szFmt, args) == nLen)
{
free(*psz);
*psz = szRes;
}
else
{
free(szRes);
nLen = -1;
}
else
nLen = -1;
}
va_end(args);
return nLen;
}
The author of the question claims there is a bug in this implementation. Not only is it a standard violation that can fail on certain esoteric systems, but a "real" error that can accidentally fail on most systems.
It also doesn't involve using a int
memory-appropriate type like size_t
or instead ptrdiff_t
. Let's say the lines are "reasonable" in size.
I really don't understand what the error is. All pointer arithmetic is in order IMHO. I don't even assume that two consecutive calls vsnprintf
give the same result. All materials with variation processing are also correct IMHO. va_copy
not required (it is the responsibility of the caller who is using va_list
). Also on x86 va_copy
and va_end
pointless.
I would appreciate it if someone can spot a (potential) bug.
EDIT:
After checking the answers and comments - I would like to add a few notes:
- Naturally, I created and ran code with various inputs, including a step-by-step debugger, observing the state of the variables. I never ask for help without trying myself first. I didn't see any problems, didn't damage the heap / heap, etc. Also I ran it in a debug build, with debug heap enabled (which is not heap portable).
- I am assuming the function is being called with valid parameters, i.e.
psz
is a valid pointer (not to be confused with*psz
),szFmt
is a valid format specifier, and all variable parameters are evaluated and matched to a format string. - The call
free
with the pointerNULL
is ok according to the standard. - The call
vsnprintf
is made using a pointerNULL
and size = 0. It should return the resulting string length. The MS version, although not completely standard, does the same in this particular case. -
vsnprintf
will not exceed the specified buffer size, including the 0-terminator. The remedy - it doesn't always place it. - Please defer the coding style (if you don't like it - great with me).
source to share
va_copy is not required (this is the responsibility of the caller who is using va_list)
Not entirely true. I have not found such a requirement for vsnprintf
in the C11 standard. He says this in a footnote:
As vfprintf, vfscanf, vprintf, vscanf, vsnprintf, vsprintf, and vsscanf call the va_arg macro, the arg value is undefined after returning .
When you call vsnprintf
, va_list
it can be passed by value or by reference (this is an opaque type for all we know). So the former vsnprintf
can actually change va_list
and destroy things for the latter. The recommended approach is to make a copy with va_copy
.
Indeed, according to this article , this does not happen on x86, but it does on x64.
source to share
The first argument to vsnprintf must not be null according to:
http://msdn.microsoft.com/en-us/library/1kt27hek(v=vs.80).aspx
Edit 1: You shouldn't free * psz if it's zero!
source to share
The first call to vsnprintf () is really an attempt to get the length of the final string. However, this has a side effect! It also moves the argument variable to the next one in the list. So the next call to vsnprintf () does not contain the first argument in the list. An easy hack is to reset the variable argument list to start over as soon as you get the length from the first vsnprintf (). Maybe there is another way to do it better, but yes, this is the problem.
source to share
Additionally, * psz can point to an already allocated string (on the heap) that could potentially be used during formatting.
For *psz
it to be potentially reusable, you must specify whether it is garbage or a valid heap pointer. If there is no function argument to indicate this, then you can accept the only sane convention for a NULL-sentinel value ... i.e. If *psz
not NULL, then you can reuse it as long as the data you want to format can fit into the same space. Since the function is not given any information about the amount of previously allocated memory, you can: - use realloc and trust it to avoid unnecessarily moving the buffer - print the minimum size of the existing buffer fromstrlen()
- this would mean that if you say that you are writing a long string, then a short string, and then the original long string to the buffer, the latter operation would uselessly replace the buffer.
Realloc is clearly the best bet.
int StrPrintF(char **psz, const char *szFmt, ...)
{
va_list args;
int nLen;
va_start(args, szFmt);
if ((nLen = vsnprintf(NULL, 0, szFmt, args)) >= 0)
{
char *szRes = (char*) realloc(psz, nLen + 1);
// ^ realloc does a fresh allocation is *psz == NULL
if (szRes)
vsnprintf(*psz = szRes, nLen + 1, szFmt, args); // can't fail
// ^ note the assignment....
else
nLen = -1;
}
va_end(args);
return nLen;
}
Note also - from the Linux manpage for printf()
- if yours sprintf()
doesn't return the usable length you got to get / write the implementation that does ....
Regarding the return value of snprintf (), SUSv2 and C99 contradict each other: when snprintf () is called with size = 0, then SUSv2 specifies an undefined return value less than 1, whereas C99 allows str to be NULL in this case, and gives a return value (like always) as the number of characters that would be written if the output string was large enough.
source to share