Reputation: 43
I'm getting some unexpected result when using vsnprintf. In the code below, I used snprintf and passed a null destination pointer to find out how much space it needs
#define long_string 256
typedef char STRING_VARIABLE [long_string + 1];
void SAFE_snprintf( char * buffer, char * format ,... )
{
va_list args;
va_start(args, format);
int m = vsnprintf (0, 0, format, args);
printf("m = %d\n", m);
if (m < 0)
{
perror ("snprintf failed");
abort ();
}
// Allocating memory
char *bufferString = (char *) malloc (n - 1);
if (!bufferString)
{
perror ("malloc failed");
abort ();
}
m = vsnprintf (bufferString, (n - 1), format, args);
if (m < 0)
{
perror ("vsnprintf failed");
abort ();
}
strcpy(buffer, bufferString);
free (bufferString);
va_end(args);
}
int main(int argc, char * argv[])
{
char InputString [] = "Hello";
STRING_VARIABLE bufferStrings;
char format [] = "%s_test";
int n = snprintf (0, 0, "%s_test", InputString);
if (n < 0)
{
perror ("vsnprintf failed");
abort ();
}
printf("n = %d", n);
SAFE_snprintf(bufferStrings, format , InputString);
return 0;
}
The above code returns
n = 7
m = 10
I'm not sure why snprintf is returning 7 (which is correct), and vsnprintf is returning 10. Which I assume is wrong or of course my understanding is flawed somewhere.
I wanted to use snprintf "safely" i.e. by avoiding string truncation. The idea was to determine the string size before using snprintf. This meant using it twice. Once to workout the size, then allocate appropriate memory to use snprintf again. Ofcourse there are a variable amount of inputs needed as any printf functions. So wanted to use vsnprintf to create a variadic function which does the aforementioned.
I know there is still the issue of checking if the original string being passed isn't too long and will not result in string truncation. But confused to why vsnprintf is not working as expected
Upvotes: 1
Views: 662
Reputation: 144949
There are multiple problems in your code:
args
without restarting the va_list
with va_start()
or va_copy()
.n - 1
bytes instead of n + 1
.strcpy
is unsafe as the composed string may exceed 256 bytes.format
should have type const char *format
.STRING_VARIABLE
is very confusing. Hiding ponters behind typedefs is confusing, but hiding arrays behind typedefs is even worse.Why not always allocate memory and return the pointer to the caller? Some C libraries have a function asprintf
with these semantics, which is a good candidate for standardization, but it took 30 years for the Committee to consider strdup()
, so making your own makes sense.
Here is an allocating version:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
// allocating version of snprintf
char *SAFE_snprintf(const char *format, ...) {
// use a local buffer to avoid calling snprintf twice for short strings
char buf[256];
va_list args;
int len;
va_start(args, format);
len = vsnprintf(buf, sizeof buf, format, args);
va_end(args);
if (len < 0) {
return NULL; // errno was set by snprintf
}
// Allocating memory
char *bufferString = (char *)malloc(len + 1);
if (!bufferString) {
return NULL; // errno was set by malloc to EMEM
}
if (len < (int)sizeof(buf)) {
strcpy(bufferString, buf);
} else {
va_start(args, format);
vsnprintf(bufferString, len + 1, format, args);
va_end(args);
}
return bufferString;
}
int main(int argc, char *argv[]) {
char InputString[] = "Hello";
int n = snprintf(NULL, 0, "%s_test", InputString);
printf("n = %d\n", n);
char *str = SAFE_snprintf("%s_test", InputString);
if (str == NULL) {
perror("SAFE_snprintf failed");
return 1;
} else {
printf("len=%zu, str=%s\n", strlen(str), str);
free(str);
return 0;
}
}
If your goal is simply to detect truncation, just compare the return value of snprintf
with the length of the destination array:
#include <stdio.h>
#include <stdlib.h>
// safe version of snprintf that detects truncation
// complain on error and truncation
// return -1 on error
// return 1 on case of truncation
// otherwise return 0
int SAFE_snprintf(char *dest, size_t size, const char *format, ...) {
int len;
va_start(args, format);
len = vsnprintf(dest, size, format, args);
va_end(args);
if (len < 0) {
perror("vsnprintf failed");
return -1;
} else
if (len < (int)size) {
// no error
return 0;
} else {
perror("vsnprintf caused truncation");
return -1;
}
}
int main(int argc, char *argv[]) {
char hello[] = "Hello";
char long_hello[] = "Pardon my intrusion, I merely want to say hello";
char name[] = "Mrs Robinson";
char buf[32];
int res;
res = SAFE_snprintf(buf, sizeof buf, "%s %s", hello, name);
printf("res=%d, buf=%s\n", res, buf);
res = SAFE_snprintf(buf, sizeof buf, "%s %s", long_hello, name);
printf("res=%d, buf=%s\n", res, buf);
return 0;
}
Upvotes: 0
Reputation: 141493
It's invalid to re-use va_list
for another call.
va_list args;
va_start(args, format);
vsnprintf(..., args);
va_end(args); // basically you have to call it
Call va_copy
or va_start
after it again before calling another v*
function.
va_list args;
va_start(args, format);
vsnprintf(..., args);
va_end(args);
va_start(args, format);
vsnprintf(..., args);
va_end(args);
or
va_list args, args2;
va_start(args, format);
vsnprintf(..., args);
va_copy(args2, args);
va_end(args);
vsnprintf(..., args2);
va_end(args2);
Your buffer is limited to 256 characters long_string 256
, so it does not solve any problem and strcpy(buffer, bufferString);
is very unsafe.
Do not use typedef
arrays - they are very confusing. Prefer a structure.
Overall, you seem to want to implement asprintf
- see sprintf() with automatic memory allocation? , https://man7.org/linux/man-pages/man3/asprintf.3.html . Maybe asprintf
is going to be standard https://en.cppreference.com/w/c/experimental/dynamic .
Upvotes: 1