pi1541 / Pi1541

Commodore 1541 emulator for the Raspberry Pi
GNU General Public License v3.0
380 stars 81 forks source link

Options might not null terminate strings #202

Open vijfhoek opened 3 years ago

vijfhoek commented 3 years ago

strncpy is used to copy options that are strings, but if say the string is "foo\0" and strncpy copies 3 characters, it'll just copy "foo", not including the null terminator. This would not be a massive problem if the char arrays were fully initialized to 0, but in the constructor only the first byte is set to 0. All in all this could lead to a non-terminated string, which could lead to some very annoying bugs :slightly_smiling_face:

I suggest one of the four following solutions:

  1. Fully initialize the char arrays to 0 by using varname{} in the constructor's initialiser list, instead of varname[0] = 0 in the constructor body.
  2. Manually setting the final byte (varname[sizeof(varname) - 1] = 0) to 0.
  3. A combination of the above
  4. Switching to std::string and std::string_view for string handling

My own preference strongly goes towards option 4, but 3 would be an okay first step.

vijfhoek commented 3 years ago

Today I learned about strlcpy, which seems to be an adequate solution for now, to at least quickly fix this issue :slightly_smiling_face: :

The strlcpy() and strlcat() functions copy and concatenate strings respectively. They are designed to be safer, more consistent, and less error prone replacements for strncpy(3) and strncat(3). Unlike those functions, strlcpy() and strlcat() take the full size of the buffer (not just the length) and guarantee to NUL-terminate the result (as long as size is larger than 0 or, in the case of strlcat(), as long as there is at least one byte free in dst). Note that a byte for the NUL should be included in size. Also note that strlcpy() and strlcat() only operate on true “C” strings. This means that for strlcpy() src must be NUL-terminated and for strlcat() both src and dst must be NUL-terminated.