Thread: Safe string copying

The code is unsafe.

std::string doesn't have a constructor that takes (char*, int, int), which is what you pass.
It has one that takes (char*, size_t), a data pointer and a length.
It has one that takes (std::string, size_t, size_t), an existing string, an offset and a length.

Because you pass three arguments, only the three-argument constructor is considered. Conversion of int to size_t is safe in this case. However, the char* is implicitly converted to a temporary std::string, and that's not only definitely unsafe (if the attacker sends no nul byte, the constructor will run off the buffer), it also doesn't work correctly, as you've found out.

Thanks, how about this:

I've changed the packet format a bit. The first byte is still the message type, then followed by 6 characters for the account number, followed by 1 to 20 characters for the password. So the minimum packet size is 8 bytes, and maximum 27 bytes.

Code:

int f(unsigned char* data, unsigned int dataLength, SystemID sourceSystem)
{
    char buff[21];
    
    if(dataLength < 8 || dataLength > 27)
    {
        std::cout << "WARNING: corrupt LOGIN_REQUEST packet received from: \n"
                  << "SystemID: " << sourceSystem << "\n"
                  << "(IP:port): " << (net->getSystemAddress(sourceSystem)).ToString(true) << "\n"
                  << (dataLength < 8 ? "packet too small" : "packet too large") << std::endl;
        return 0;
    }

    strncpy_s(buff, 21, reinterpret_cast<char*>(data + 1), 6);
    std::string acc(buff);
    
    strncpy_s(buff, 21, reinterpret_cast<char*>(data + 1 + 6), dataLength - 7);
    std::string pass(buff);

// testing only:
    std::cout << acc << " " << acc.length() << "\n" << pass << " " << pass.length()
              << " " << dataLength << std::endl;
    return 0;
}

Well, now it's unportable, but at least it's safe.

Shouldn't you return something different on error than on success?

strncpy() is reasonable portable (C99) and safe enough for this purpose. For the account number, which is of a fixed size, I'd just use memcpy() and manual termination.