Fix base64_encode including '\0' in output.

[puetzk]

CryptBinaryToStringA([in, out] pcchString) has unusual behavior

If pszString is NULL, the function calculates the length of the return
string (including the terminating null character) in TCHARs

If pszString is not NULL and big enough, the function converts the
binary data into a specified string format including the terminating null
character, but pcchString receives the length in TCHARs,
not including the terminating null character

So it requests a buffer size with room for a terminating '\0', and will write that terminator, but won't count it in the final length. And this std::string likewise implies a '\0'-termination, but doesn't consider that part of std::string::size().

The terminator written by CryptBinaryToStringA shouldn't be left as part of the content of str.

---

Currently this is causing winsparkle-tool to print embedded '\0 characters in the output of public-key, sign, etc. Which isn't visible in most terminals, but you can see it if you do e.g. winsparkle-tool ... | xxd, and it can mess up if scripts capture the output.

[vslavik]

Thanks for the PR and detailed explanation!

However, there's no point in always resizing the string and calculating the test, which is guaranteed to evaluate to true. This off-by-one error is best fixed at its source with a two-characters change:

-    std::string str(base64_len, '\0');
+    std::string str(base64_len - 1, '\0');

So that's what I did instead in 5e2ad7c.

[puetzk]

I considered that, but didn't like it because CryptBinaryToStringA does write the trailing `'\0', so .size() woujld be one byte short of the length it will write. I thought that would be undefined behavior.

But looking at https://en.cppreference.com/w/cpp/string/basic_string/data.html in more detail, my concern would have been (pedantically) right in C++03, but was redefined as safe since c++11. The standard now says the .data() must include the same terminating null as .c_str() and point to a closed interval (i.e. the implied null at data() + size() is safe to dereference, and always contains a null character. Writing is also called out:

2. Modifying the past-the-end null terminator stored at data() + size() to any value other than CharT() has undefined behavior.

As you note in your commit message, CryptBinaryToStringA will be writing a zero, which matches CharT(). So yep, your fix is simpler and sound given modern c++. Which winsparkle-tool code already needs: non-const .data() is c++17. Even the oldest toolset in WinSparkle.vcxproj list is v110 == visual studio 2012 would define the closed interval (just as const).

Soo... long way of saying "looks good to me" (and I learned something).

[vslavik]

and I learned something

Me too, on basically the same route as you :)