14
\$\begingroup\$

I know that stringstreams are the C++ recommended way to create formatted text. However, they can often become quite verbose, especially when compared to the succinct format strings of printf and family. However, the printf family can lead to its own issues, with buffer overflow issues, and so I would rather not use these functions directly

My goal was to make a function to behave similarly to snprintf, but return a std::string, avoiding any chance of buffer overflow. Knowing that there are many pitfalls to string manipulation in C++, have I exposed myself to any errors in using this function?

#include <cstdio>
#include <cstdarg>
#include <string>

std::string string_sprintf(const std::string& format, ...){
  static const int initial_buf_size = 100;
  va_list arglist;
  va_start(arglist, format);
  char buf1[initial_buf_size];
  const int len = vsnprintf(buf1,initial_buf_size,format.c_str(), arglist) + 1;
  va_end(arglist);

  if(len<initial_buf_size){
    return buf1;
  } else {
    char buf2[len];
    va_start(arglist,format);
    vsnprintf(buf2,len,format.c_str(),arglist);
    va_end(arglist);
    return buf2;
  }
}
\$\endgroup\$
6
\$\begingroup\$

Since you're using cstdarg and va_list is a complete type, it should be std::va_list.


va_start with a std::string is undefined behavior. This means that you actually cannot use std::string as your format (at least not if you plan on using only the stdargs facilities).

It's literally impossible, since there's no way to forward the ... idea, but instead you can only forward a va_list, which of course presents a problem since you can't obtain a va_list with a std::string parameter.


Using vsnprintf implies that you're using C++11. Given this, I might be tempted to go the variadic template route rather than leveraing the old C style functions that don't have type safety. Unfortunately though, much of the parsing and transforming logic would have to be recreated. If you do want to consider going this route, Andrei Alexandrescu gave a talk in which he examined this option.

Note that using a std::string format with variadic templates is of course possible.

Oh, and of course you could do a hybrid where you use a variadic template, but only to forward the arguments to vsnprintf. That would allow you to use a std::string format but you don't get the true typesafety of the variadic approach.


vnp already covered this, but let's take a second look and consider an excerpt from the standard:

The vsnprintf function returns the number of characters that would have been written had n been sufficiently large, not counting the terminating null character, or a negative value if an encoding error occurred. Thus, the null-terminated output has been completely written if and only if the returned value is nonnegative and less than n.

Unfortunately, you'll have to do some pretty gross handling for this since on MS systems a negative number currently means the buffer was not large enough whereas on standard compliant systems it means some of encoding error happened.


If you include the null character in the calculation, you need to use <=. If you don't include it, you need to use <. This means that your current logic of len < initial_buf_size is wrong. It should actually be len <= initial_buf_size.

Think of it this way: len represents the length of the entire, null terminated output string (since you added 1). initial_buf_size represents the size of the entire buffer, including the required null terminator. Since both numbers include the null terminator, the case where they are equal means that the entire buffer was utilized but that it does contain the full, null terminated output string.


I wouldn't bother using initialize_buf_size. Instead, I would just hard code it into the buf1 declaration and then use sizeof(buf1). I'm all for avoiding magic numbers and pulling them into constants, but it's really only used once (and you should be using sizeof anyway -- the declaration of buf1 can change).


2 spaces is rather unusual in C++. I would expect either 4 spaces or 1 tab.


I'm not a fan of the lack of white space. Like the previous item, this is just opinion, but I (and I think most people -- though obviously I'm prone to confirmation bias on this) prefer spacing around clauses and operators. In other words:

func(param1, param2, param3)

and

if (a < b) {

Your function has way too much duplicated code. Let the va_list function be handled at the top level, and then have a second function that actually does the work. This is actually exactly what every implementation of the standard library I've ever used does with.

Basically what I'm saying is that just as the standard library has printf and vprintf, you would have string_sprintf and string_vsprintf, and string_sprintf would just call string_vsprintf under the hood.


Instead of having two cases, I would be tempted to just do an empty run to vsnprintf to figure out exactly what size you'll need.


You're using a non-standard compiler extension to use a automatic memory duration array that is actually dynamically sized (buf2).

Instead, I would use a container like a std::vector to handle the buffer, or since you're arleady using C++11, you can use std::string since it's guaranteed to be contiguous.


All in all, I might do something like this:

std::string string_vsprintf(const char* format, std::va_list args) {
    va_list tmp_args; //unfortunately you cannot consume a va_list twice
    va_copy(tmp_args, args); //so we have to copy it
    const int required_len = vsnprintf(nullptr, 0, format, tmp_args) + 1;
    va_end(tmp_args);

    std::string buf(required_len, '\0');
    if (std::vsnprintf(&buf[0], buf.size(), format, args) < 0) {
        throw std::runtime_error{"string_vsprintf encoding error"};
    }
    return buf;
}

std::string string_sprintf(const char* format, ...) __attribute__ ((format (printf, 1, 2)));

std::string string_sprintf(const char* format, ...) {
    std::va_list args;
    va_start(args, format);
    std::string str{string_vsprintf(format, args)};
    va_end(args);
    return str;
}

Note that this does not handle Windows and MS's non-compliant implementation of vsnprintf.

It would also be a good idea to wrap the __attribute__ stuff in preprocessor checks since __attribute__ is only used in GCC and clang.

Oh, and the string is unnecessarily set to all null characters and only then over written with the data we actually care about. If you super care about performance, that might be an issue, but realistically, it shouldn't matter. Anyway around it would require an extra copy anyway (since you'd have to take whatever buffer you used and copy it into a std::string).

\$\endgroup\$
6
\$\begingroup\$

My main problem is that it is as bad as fprintf(). It provides exactly no type safety. There is no validation between the format string and the parameters I pass as arguments. In fact fprintf() is safer as most compilers have extensions that actually validate the parameter types against the format string (that will not happen here).

Second problem it provides no support for user defined types (or standard C++ types). So it provides no extra compactness when I use other types.

Third I am not in love with returning a string. What happens if I want to pass values to a stream. This means I have to inefficiently build a string (which can grow large), return that then pass it to the stream I want to place it on.

Personally I would pass a stream (and return it). That way if I wanted internal string formatting I pass std::stringstream. If I want a file I pass a std::ofstream if I want to send it over a socket I pass a socket stream etc.

// My interface to solve all these problems would be.
template<typedef...T>
std::ostream& string_sprintf(std::ostream& out, char const* format, T... const& param);


// Usage:
string_sprintf(std::cout, "Hi %i ", 5) << "And chain other stuff";
\$\endgroup\$
5
\$\begingroup\$

Beware that vsnprintf may return -1. It is a non-compliant implementation, yet widely used.

I honestly don't see a need to pass a format parameter as std::string. It is only used as c_str(); besides, passing it as const char * enables using __attribute__ ((format (printf, 1, 2))) for compilers that supports it.

\$\endgroup\$
  • \$\begingroup\$ I did not know about the different behavior of vsnprintf. Thank you. I used a std::string as input so that I would be able to have both input and output be strings, rather than char*. I was unaware of the __attribute__ flags, and that is a good reason for me to change it. Edits on both accounts should be made shortly. \$\endgroup\$ – Eldritch Cheese Jun 5 '14 at 17:38
4
\$\begingroup\$

Allocating a buffer of arbitrary size on the stack is not safe — it can easily cause a stack overflow. This happens at about 8 MB on my 32-bit Linux machine, which is plausible for some uses of string_sprintf.

If your application needs large strings, you'll need to allocate the space on the heap (perhaps by string::reserve and string::data). If not, you should at least sanity-check len in case some huge string finds its way into string_sprintf.

\$\endgroup\$
  • \$\begingroup\$ Just cahnge char buf2[len]; into std::vector<char> buf2(len); Pointer for use in vsnprintf() can be retrieved via &buf[0] \$\endgroup\$ – Martin York Jun 6 '14 at 10:15
0
\$\begingroup\$

Here is what I've been using recently:

size_t safe_stringprintf(std::string& str, const char *fmt, ...) {
    va_list ap;
    size_t l;
    va_start(ap, fmt);
    if((l = snprintf(const_cast<char *>(str.data()), str.size(), fmt, ap)) > str.size()) {
        str.reserve(l);
        snprintf(const_cast<char *>(str.data()), str.size(), fmt, ap);
    }
    va_end(ap);
    return l;
}

If I understand correctly, it's casting away the const of a variable declared const which is undefined, whereas .c_str and .data are views to a non-const buffer. I'm not sure, though, whether this can cause undefined behavior. It does seem to work properly.

At least one performance advantage here is that one doesn't need to copy from the char[] buffer to the final std::string.

\$\endgroup\$
  • \$\begingroup\$ Ah, neat. I think that whether or not you invoke undefined behavior depends on the version of C++. In C++11, std::string is guaranteed to have a contiguous array of memory, whereas in C++03, it is not. You could also use &str[0] instead of str.data(), since string::operator[] returns a nonconst reference. \$\endgroup\$ – Eldritch Cheese Mar 21 '16 at 13:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.