Use snprintf instead of sprintf (even in trivial case)

[eddelbuettel]

This commit replace the sole use sprintf with snprintf. A full run of reverse depends checks revealed no 'change to worse'.

Checklist

• Code compiles correctly
• R CMD check still passes all tests
• Prefereably, new tests were added which fail without the change
• Document the changes by file in ChangeLog

[kevinushey]

snprintf() always writes a null terminator as well, so I think you can safely pass the full buffer size when calling it (no -1).

[kevinushey]

Do we have a unit test for this one? I'm worried we might be truncating the representation here.

[eddelbuettel]

I would welcome one. Can you think of one?

I did run it over nearly 2600 CRAN packages though as the usual reverse depends check...

[kevinushey]

I'll try to put one together later this morning.

[kevinushey]

Here's an example showcasing the issue:

#include <Rcpp.h>
using namespace Rcpp;

// [[Rcpp::export]]
std::string test() {
    return Rcpp::internal::coerce_to_string<RAWSXP>(0xaa);
}

/*** R
test()
*/

Running with this PR, you'll see:

> test()
[1] "a"

Given that this change doesn't cause any failures I guess this isn't a commonly-exercised code path.

[eddelbuettel]

Whoops. Scrambled egg on my face.

[eddelbuettel]

snprintf() always writes a null terminator as well, so I think you can safely pass the full buffer size when calling it (no -1).

I was a little fuzzy / unaware on this one but do recall seeing many use of -1 in the wild. Including in the same source file a few lines above:

Rcpp/inst/include/Rcpp/internal/r_coerce.h

Lines 240 to 241 in b3b9bd9

	static char tmp[128];
	snprintf(tmp, 127, "%f", x);

but then not here (so I changed it in the PR -- the link shown is from the master branch)

Rcpp/inst/include/Rcpp/internal/r_coerce.h

Lines 248 to 249 in 4015d89

	static char buffer[NB] ;
	snprintf( buffer, NB, "%*d", integer_width(from), from );

What is a good rule about when 'n == n' is safe and when it is not?

[eddelbuettel]

Rebased and updated. Let me know what you think @kevinushey

[kevinushey]

What is a good rule about when 'n == n' is safe and when it is not?

Good question... it's complicated and comes down to whether the format / print routine writes a null terminator for you or not. Since snprintf() always writes a null terminator, it's safe to use n == n where the buffer includes space for the null terminator, but for other formatting functions the documentation needs to be checked to be sure.

Another wrinkle for snprintf() is that, if you're using it to figure out the buffer size when printing, you need to be careful. From https://en.cppreference.com/w/cpp/io/c/fprintf:

4. Writes the results to a character string buffer. At most buf_size - 1 characters are written. The resulting character string will be terminated with a null character, unless buf_size is zero. If buf_size is zero, nothing is written and buffer may be a null pointer, however the return value (number of bytes that would be written not including the null terminator) is still calculated and returned.

So you might write code like:

// figure out size of formatted string
// NOTE: does _not_ include null terminator!
int n = snprintf(nullptr, 0, "%s\n", "code");

// create a buffer, adding 1 for the null terminator
std::vector<char> buffer(n + 1);

// format string into buffer, again adding 1 for terminator
snprintf(&buffer[0], n + 1, "%s\n", "code");

tl;dr: it's unfortunately complicated and depends on whether the formatter writes a null terminator for you, and in what conditions it writes the null terminator.

[kevinushey]

This is also why I am so happy to have fmt.

[kevinushey]

LGTM!

[eddelbuettel]

Thanks for the comment above. Reading it I realize I once knew only n-1 may be copied to leave room. So yes, 'it depends'.

Anyway, we made a nice improvement and you rightly took me to school as there shouldn't be a PR without a test under it. All good.