Perform copy only if value has size > 0.

[space88man]

Addresses #449.

When value has size 0, the invalid reference &byteString[0] in ByteString::const_byte_str will cause an assertion when built with the flag -Wp,-D_GLIBCXX_ASSERTIONS (used by default in distros like Fedora)

[space88man]

Did you build with -Wp,-D_GLIBCXX_ASSERTIONS on Fedora?

[halderen]

No, I did not, but was busy doing so. With that flag, the error occurs not just on Fedora, but on most (all?) platforms. I think it might be wise to add the flag when building on our test platforms, so we can catch these errors.
Thanks for digging in and getting this flag.

[space88man]

Fedora also uses -Wp,-DFORTIFY_SOURCE=2

https://access.redhat.com/blogs/766093/posts/1976213

[space88man]

What about solving the issue in ByteString.cpp?

If byteString.size() != 0 return &byteString[0] else return NULL

Alternatively , if we don't want to ever get a NULL back (that might break existing callers) some sentinel value in the translation unit

// if size() == 0 then return a pointer to this sentinel value
unsigned char sentinel[1]; //somewhere inside ByteString.cpp

The second commit here is a POC.

update: rstudio/rstudio#3677 uses .data() but if size() is 0, data() may or may not return a null pointer.

[space88man]

Some discussion from @halderen, @rijswijk lost due to a forced push, recording here:

---

@halderen: "I was more looking at a similar patch using
return byteString.data();
Which is protected against a zero sized string."

---

@rijswijk: "if std::vector.data() can be cast to either unsigned char* or const unsigned char* that is probably the best solution (we may have originally used that and changed it to &byteString[0] later because that cast cannot be made on every platform, but it's too long ago to remember. The STL description says that in case of size() == 0 then .data() may or may not return a NULL pointer."

---

@rijswijk: "Scrap that, we didn't try the .data() approach, since that didn't appear until the C++ 11 standard according to the STL docs. This could be a consideration, it may break builds on pre- C++ 11 systems."

[halderen]

I think we can use the .data() call as discussed earlier, since C++11 has become the norm for most. However I will pull in the issue as-is for now, perhaps indeed changing it into .data() later in order to make sure this fix will get into the next release in one way or another.