C considered dangerous

Posted Aug 30, 2018 6:59 UTC (Thu) by anton (subscriber, #25547)
In reply to: C considered dangerous by iabervon
Parent article: C considered dangerous

I'm not sure another argument to memcpy would help; unlike with strcpy, you have the number of bytes that would be copied anyway.

Yes. So now the programmer has two numbers, and only one parameter. What will he do? Will he write a long sequence that takes both numbers into account? Or will he just call memcpy() with the source length, because he knows that the destination buffer is bug enough? Maybe he is right in his knowledge, and maybe that knowledge does not become wrong during maintenance. But would you bet your program's security on that?

Better give him a library function with two length parameters, and an appropriate reaction (probably reporting an error) if the destination buffer is too small.

C considered dangerous

Posted Aug 30, 2018 7:47 UTC (Thu) by rschroev (subscriber, #4164) [Link]

> ... because he knows that the destination buffer is bug enough

Well played, sir :)

C considered dangerous

Posted Aug 31, 2018 20:46 UTC (Fri) by rweikusat2 (subscriber, #117920) [Link] (8 responses)

I'm sorry but this is nonsense. memcpy copies a fixed number of bytes from a source buffer to a destination buffer. This means it's up to other code to ensure that the destination buffer is large enough. Considering that the number of bytes which will be copied is necessarily known, that's easy enough to do. This is a code correctness issue and the bug (buffer to small) doesn't go away because memcpy compares some random numbers.

I mean, your fictional programmer who can't be trusted to get something as simple as that right suddenly can be trusted to always pass another parameter reflecting the actual size of the buffer correctly? How so? Why shouldn't he just pass (size_t)-1 because he knows "the buffer will be big enough", IOW, doesn't care?

C considered dangerous

Posted Sep 1, 2018 14:04 UTC (Sat) by epa (subscriber, #39769) [Link] (7 responses)

Yes, a sloppy and careless programmer will write dangerous code with memcpy_s() or any other routine. And the elite-level programmer will use memcpy() and do so safely, because he or she has carefully tracked the sizes of all buffers and not made any mistakes. It’s the average schmoes in the middle, who do our best but are fallible, who can be helped by an interface that takes some redundant information.

C considered dangerous

Posted Sep 3, 2018 15:49 UTC (Mon) by rweikusat2 (subscriber, #117920) [Link] (6 responses)

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.