C considered dangerous

Posted Sep 3, 2018 15:49 UTC (Mon) by rweikusat2 (subscriber, #117920)
In reply to: C considered dangerous by epa
Parent article: C considered dangerous

All humans are fallible (and all computer are prone to mechanical defects:-) but that's besides the point.

The interesting question is "Does one gain anything tangible by passing a destination buffer size to a block memory copy routine which already takes a number-of-bytes-to-copy parameter" and the answer is no: The application programmer still has to ensure that the buffer whose address is passed is large enough so that the intended number of bytes can be copied into it. Ie, the only change is that the programmer now has to track two lengths instead of one. Assuming that tracking one length correctly is already considered too difficult for people to do reliably, how can the situation possibly be improved by introducing another length value application programmers have to worry about?

C considered dangerous

Posted Sep 5, 2018 8:56 UTC (Wed) by anton (subscriber, #25547) [Link] (5 responses)

Let's take a look at an example:

char buf[BUFLEN];

And we want to copy len bytes from src to buf, and we know that len<=BUFLEN. Doing the copy with

memcpy(buf,src,len);

would be correct, as would be

memcpy_s(buf,BUFLEN,src,len);

Now, during maintenance, there are changes that can cause len>BUFLEN. Now the first variant has a buffer overflow, while the second does not (but, as written, does not report the error). Tracking BUFLEN and len is not the problem here; it's tracking whether the assumption implied in the memcpy-using code is true.

The tangible benefit that one gets with the memcpy_s variant is that we do not get a buffer overflow even if there is a mistake in tracking whether the assumption is true. True, we do not get that benefit if we write the wrong value as the second parameter of memcpy_s, but that's a less likely mistake in this case, and probably in many others.

You can make the assumption explicit by writing

assert(len<BUFLEN);

before the memcpy, but then you have to "track two lengths" again, and in addition, assertion checking can be disabled, or the length in the memcpy can be changed without changing the assertion.

C considered dangerous

Posted Sep 5, 2018 9:08 UTC (Wed) by johill (subscriber, #25196) [Link] (3 responses)

To be fair, in your example the size of the buffer could also be changed without changing BUFLEN (which is your objection to the assert), so you should write

memcpy_s(buf,sizeof(buf),src,len);

in this case, I guess.

C considered dangerous

Posted Sep 5, 2018 9:23 UTC (Wed) by anton (subscriber, #25547) [Link]

Yes, that's even better. I was actually thinking about the maintenance programmer changing the len parameter of memcpy (to, say, len+a-b), and forgetting to change the assertion; there are often stale comments, so I would not be surprised about stale assertions (unless the tests cause them to trigger).

C considered dangerous

Posted Sep 5, 2018 10:12 UTC (Wed) by excors (subscriber, #95769) [Link]

Just be careful if someone might move that into a separate function like "void do_copy(char buf[BUFLEN], ...) { memcpy_s(buf, sizeof(buf), ...); }", and either ignores compiler warnings or is using an older compiler, since sizeof(buf) is now 8 regardless of BUFLEN. C is fun. Better to use a decent ARRAY_SIZE macro (like the one in the Linux kernel) that fails to compile if the argument is not an array.

C considered dangerous

Posted Sep 5, 2018 10:21 UTC (Wed) by pizza (subscriber, #46) [Link]

Unless, of course, 'buf' is just a raw pointer -- then sizeof(buf) won't give you what you want.

(Personally, most uses of 'memcpy' in my code involve assembling chunks of data into a buffer. Nearly all of the memcpy()s' destinations are at nonzero offsets of the original buffer, so memcpy_s() really doesn't do me any good...)

C considered dangerous

Posted Sep 5, 2018 20:12 UTC (Wed) by rweikusat2 (subscriber, #117920) [Link]

Well, for the sake of the contrived example, let's assume that "during maintenance",

memcpy_s(buf, BUFLEN, src, len)

is changed to

memcpy_s(src, len, buf, BUFLEN)

now, there's no buffer overflow but likely a very serious memory corruption problem.

There's exactly no reason for this assumption except that's it's another way to break the working code but there wasn't any reason for your assumption, either.