4 ms·

I don’t understand why the “lessons learned” doesn’t recommend always* passing the destination buffer size (using memcpy_s or your own wrapper). It has been a

I don’t understand why the “lessons learned” doesn’t recommend always* passing the destination buffer size (using memcpy_s or your own wrapper). It has been a long time since I wrote C++, but when I did this would have been instantly rejected in code review. *…with, I suppose, potential exceptions in performance-critical code when you control and trust the input; I don’t believe that this code qualifies on either count.

rfoo 5y ago
That's because these are "lessons learned" for how to catch these bugs, instead of "how to write more secure code". Because you can't.
- jmull 5y ago
  You catch the bug by flagging the use of memcpy instead of something that takes the dest buffer size (like memcpy_s or whatever). It seems to me linters have been flagging this kind of thing since forever. This code is using a wrapper, "PORT_memcpy", so a default ruleset isn't going to flag it. So here I guess no one noticed PORT_memcpy == memcpy (or maybe noticed but didn't take the initiative to add a lint rule or deprecation entry or just created an issue to at least port existing code).
  - galangalalgol 5y ago
    was no one linting the wrapper? the static analysis tools we use wouldn't like memcpy_s either. It would create a finding to use an stl container probably.
AnimalMuppet 5y ago
Counterexample: msgrcv(). This expects you to not be passing raw buffers, but messages with a particular structure: a long mtype, to specify what type of message it is, and then a char (byte, since this is C) array that is the buffer that contains the rest of the message. You pass these structures to msgsnd() and msgrcv(), along with a size. But the size is the size of the buffer component of the structure, not the size of the structure as a whole. If you pass the size of the structure, it will read sizeof(long) more than your structure can hold. Been bit by that... So, just passing the size of the destination is something that you can still get wrong, in the case of data more complicated than just a single buffer. [Edit: You can also design an API to be very misleading, even if it has a length parameter...]
ergl 5y ago
It's not the case here (I think), but this can be common if you move functions around or expose functions that were previously internal. For example, maybe your function is only called from another one that performs the appropriate bound checks, so checking again becomes redundant. After a simple refactoring, you can end up exposing your function, and screw up.

rfoo 5y ago

That's because these are "lessons learned" for how to catch these bugs, instead of "how to write more secure code". Because you can't.

jmull 5y ago
You catch the bug by flagging the use of memcpy instead of something that takes the dest buffer size (like memcpy_s or whatever). It seems to me linters have been flagging this kind of thing since forever. This code is using a wrapper, "PORT_memcpy", so a default ruleset isn't going to flag it. So here I guess no one noticed PORT_memcpy == memcpy (or maybe noticed but didn't take the initiative to add a lint rule or deprecation entry or just created an issue to at least port existing code).
- galangalalgol 5y ago
  was no one linting the wrapper? the static analysis tools we use wouldn't like memcpy_s either. It would create a finding to use an stl container probably.

AnimalMuppet 5y ago

Counterexample: msgrcv(). This expects you to not be passing raw buffers, but messages with a particular structure: a long mtype, to specify what type of message it is, and then a char (byte, since this is C) array that is the buffer that contains the rest of the message. You pass these structures to msgsnd() and msgrcv(), along with a size. But the size is the size of the buffer component of the structure, not the size of the structure as a whole. If you pass the size of the structure, it will read sizeof(long) more than your structure can hold. Been bit by that... So, just passing the size of the destination is something that you can still get wrong, in the case of data more complicated than just a single buffer. [Edit: You can also design an API to be very misleading, even if it has a length parameter...]

ergl 5y ago

It's not the case here (I think), but this can be common if you move functions around or expose functions that were previously internal. For example, maybe your function is only called from another one that performs the appropriate bound checks, so checking again becomes redundant. After a simple refactoring, you can end up exposing your function, and screw up.