Safe strcpy implementation with C++

Question

After reading this article, I've decided to take a crack at implementing a "safe_strcpy" function that would remove some of the errors that the author speaks about. Obviously, my safe_strcpy() function is inspired by his. Below, I've created a simple class that houses a std::array of one of the many different kinds of strings. My safe_strcpy() functions rely on always null-terminating the array given.

I have a few worries about my implementation:

1. The lines with out[N - 1] = static_cast<T>(0) seem wrong when the underlying character type is wchar_t. In this sense, I want the array to terminate with something to the effect of L"0". Will this work? If not, how can I fix it?

2. Should I be concerned with any performance issues or any "gotchas" that I didn't think of?

3. Is this a viable solution to the safe_strcpy conundrum? In other words, would you use this class in production code? What kind of improvements or extensions should this class use?

#include <cstdlib>
#include <cstdio>
#include <cstring>
#include <array>
#include <iostream>
#include <typeinfo>
using namespace std;

template <size_t N, class T = char>
struct char_array
{
    typedef T                value_type;
    typedef const T          const_value_type;
    typedef T&               reference;
    typedef const T&         const_reference;
    typedef std::array<T, N> container;

    typedef typename container::size_type       size_type;
    typedef typename container::iterator        iterator;
    typedef typename container::const_iterator  const_iterator;

    typedef typename container::reverse_iterator        reverse_iterator;
    typedef typename container::const_reverse_iterator  const_reverse_iterator;

    /* Constructor */
    char_array()
    {
        static_assert((is_same<unsigned char, T>::value || is_same<char, T>::value ||
                      is_same<wchar_t, T>::value || is_same<signed char, T>::value ||
                      is_same<char16_t, T>::value || is_same<char32_t, T>::value)  &&
                      N > 0,
                      "char_array initialized with invalid type or size");
    }

    /* Destructor */
    ~char_array() {}

    /* Copy constructor and assignment */
    char_array(const char_array<N,T> &other) = default;
    char_array& operator=(const char_array<N,T> &other) = default;

    /* Move constructor and assignment */
    char_array(char_array<N,T> &&other) { buf_ = std::move(other.buf_); }
    char_array& operator=(char_array<N,T> &&other) 
    { 
        buf_ = std::move(other.buf_);
        return *this;
    } 

    /* Delegate members of internal container */

    iterator         begin() { return buf_.begin(); }
    const_iterator   begin() const { return buf_.begin(); }

    iterator         end() { return buf_.end(); }
    const_iterator   end() const { return buf_.end(); }

    const_iterator   cbegin() const { return buf_.cbegin(); }    
    const_iterator   cend() const { return buf_.cend(); }

    reverse_iterator         rbegin() { return buf_.rbegin(); }
    const_reverse_iterator   rbegin() const { return buf_.rbegin(); }

    reverse_iterator         rend() { return buf_.rend(); }
    const_reverse_iterator   rend() const { return buf_.rend(); }

    const_reverse_iterator   crbegin() const { return buf_.crbegin(); }
    const_reverse_iterator   crend() const { return buf_.crend(); }

    reference       operator[](size_type i) { return buf_[i]; }
    const_reference operator[](size_type i) const { return buf_[i]; }

    size_type        size() const { return buf_.size(); }
    size_type        max_size() const { return buf_.max_size(); }
    bool             empty() const { return buf_.empty(); }

    reference        at(size_type n) { return buf_.at(n); }
    const_reference  at(size_type n) const { return buf_.at(n); }

    reference        front(size_type n) { return buf_.front(n); }
    const_reference  front(size_type n) const { return buf_.front(n); }

    reference        back(size_type n) { return buf_.back(n); }
    const_reference  back(size_type n) const { return buf_.back(n); }

    value_type*       data() { return buf_.data(); }
    const value_type* data() const { return buf_.data(); }

    void fill(const value_type &val) { buf_.fill(val); }
    void swap(char_array &x) { buf_.swap(x.buf_); }

private:
    container buf_;
};

/*
    Allow "template magic" to automatically deduce
    the size of the given array. NOTE: this only works with
    statically-allocated arrays (e.g. arrays with a size known
    at compile time)
    @param[in] - out - a reference to the array
    @param[in] - src - the source string

    Usage:
    char buf[10];
    safe_strcpy(buf, "This is a string > 10 chars");
 */
template <class T, size_t N>
void safe_strcpy(T (&out)[N], const T *src)
{
    static_assert((is_same<wchar_t, T>::value || is_same<signed char, T>::value ||
                is_same<unsigned char, T>::value || is_same<char, T>::value || 
                is_same<char16_t, T>::value || is_same<char32_t, T>::value)  &&
                N > 0, "Incompatible type for safe_strcpy");
    memcpy(out, src, N * sizeof(T));
    out[N - 1] = static_cast<T>(0);
}

template <class T, size_t N, class S>
void safe_strcpy(char_array<N,T> &out, const S *src)
{
    static_assert(is_same<T, S>::value, "Source content type different from char_array destination");
    memcpy(out.data(), src, N * sizeof(T));
    out[N - 1] = static_cast<T>(0);
}

template <class Os, size_t N, class T>
Os& operator<<(Os &os, const char_array<N,T> &arr)
{
    for (const auto &c : arr) os << c;
    return os;
}

int main()
{
    wchar_t buf[10];
    safe_strcpy(buf, L"really long string that's greater than 10 freaking characters!!!!");
    wcout << buf << "\n";

    char_array<10> buffer;
    safe_strcpy(buffer, "This is a string longer than 10 chars");
    cout << "Max size is: " << buffer.max_size() << "\n";
    cout << buffer << "\n";
}

Answer 1

Break out your type trait

You have this large amount of is_same checks in a couple places. Break it out in its own trait, and call it something like is_char:

template <class T> is_char : std::false_type { };
template <> is_char<wchar_t> : std::true_type { };
template <> is_char<char> : std::true_type { };
template <> is_char<signed char> : std::true_type { };
...

What way, in your char_array, you can just have:

static_assert(is_char<T>::value, "invalid type, expected a char");
static_assert(N > 0, "invalid size, expected N > 0");

Furthermore, those asserts don't need to be in the constructor, they can be in the body of the class.

Lots of code Repetition

Pretty much all of your char_array code is just duplicating what std::array does. What does char_array actually give you? It costs you aggregate-initialization, and that's not nothing. Ultimateyl, it's really just a std::array<> with some extra conditions on T and N.

But we can just use those same conditions to SFINAE the safe_strcpy:

template <class T, size_t N, class S,
          class = std::enable_if_t<is_char<T>::value &&
                                   (N > 0) &&
                                   is_same<T, S>::value>>
void safe_strcpy(std::array<T,N> &out, const S *src)
{
    memcpy(out.data(), src, N * sizeof(T));
    out[N - 1] = 0;
}

And now we have no need for char_array at all.

---

If you really want to keep the type, I'd just inherit:

template <size_t N, class T=char>
struct char_array : std::array<T, N>
{
    static_assert(is_char<T>::value, "invalid type, expected a char");
    static_assert(N > 0, "invalid size, expected N > 0");
};

memcpy to std::copy

Instead of copying raw bytes, and having to remember to multiply, you can just use std::copy - which will under-the-hood do the same thing anyway, but will be less error-prone. So instead of:

memcpy(out.data(), src, N * sizeof(T));

Write:

std::copy(src, src+N, out.data());

Answer 2

• Gotcha

  There is a potential for illegal access. While the code guarantees that the destination buffer is not overflown, it will however access beyond the source buffer if it is smaller than the destination. memcpy is just a wrong tool here.

• Will I use it?

  Given a very limited use case of this approach (static buffers only) and a template size specialization (a serious potential for code bloat) I will probably stay with the good old strncpy. Rumors of its un-safety are highly exaggerated.

Answer 3

One hard cold truth is that C-Strings cannot be fixed: not knowing the size of the buffer (either inbound or outbound) is a recipe for failure, and the band-aids (strncpy, strlcpy, ...) are just attempting to patch the symptoms.

Another hard cold truth is that wchar_t is broken by design as it is non-portable (16 bits on Windows and 32 bits on Unix). Therefore you should not use it at all and instead rely on explicitly sized types char16_t and char32_t.

---

Your own code is subject to run-time issues as a result:

• you never guarantee that src is not null, at the very least an assert would be useful and the documentation should reflect it, some compilers also have specific annotations (gcc's __attribute__((nonnull(2))) for example)
• you never check that src is long enough and blindly copy N bytes, even though in general there is no guarantee that the source is longer than the destination

And has a number of design limitations:

• you do not allow copying piecemeal (useful to concatenate...)
• you only allow copying into statically allocated buffer
• you treat the strings as a meaningless sequence of bytes, and truncate without regard for its encoding (but then, std::string does so too...)

---

If you cannot afford std::string, or wish to design an alternative where buffer ownership can be external (so as to use fixed-sized buffer), then I advise using a class approach and simply make it compatible with C-Strings for interoperability:

• by allowing construction from a C-String
• by keeping it NUL-terminated for cheap conversion into a C-String

And then, this class can easily implement safe copy!

Last note: std::string may not be as costly as you think it is, if you are avoiding it for performance reason, measure first...

A rough example (untested!) of a safer alternative to C-String:

//
//  Fixed-Capacity String
//
//  It never allocates or re-allocates, but instead uses the
//  externally provided buffer.
//
template <typename T>
class fcstring {
public:
    //  Construction
    explicit fcstring(T* cstring):
        _capacity(cstring ? std::strlen(cstring) : 0),
        _size(_capacity),
        _buffer(cstring)
    {}

    template <size_t N>
    explicit fcstring(T (&str)[N]):
        _capacity(N-1),
        _size(N-1),
        _buffer(str)
    {}

    //  "buffer" is assumed to already point at a valid C-String
    //  of size "size" (not counting the terminating NUL-byte).
    //  "capacity" may be additionally provided if the buffer is known
    //  to be larger than "size", it then represents the number of "T"
    //  than the "buffer" may contain, not counting the NUL-byte.
    fcstring(T* buffer, size_t size, size_t capacity = 0):
        _capacity(capacity ? capacity : size),
        _size(size),
        _buffer(buffer)
    {}

    //  Copy
    void assign(fcstring const& other) {
        size_t const copied = std::min(_capacity, other._size);
        std::memcpy(_buffer, other._buffer, copied);
        _buffer[copied] = T();
        _size = copied;
    }

    //  Conversion to C-String
    //  /!\ Be mindful that any embedded NUL character will
    //  /!\ result in a truncated output
    T const* c_str() const { return _size ? _buffer : ""; }

private:
    size_t _capacity;
    size_t _size;
    T* _buffer;
}; // class fcstring