Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

For any junior engineers keeping score at home:

NEVER WRITE THIS CODE! EVER!

This is the code that gives us the classic buffer overflow vulnerability all because checking for a length isn't elegant.

Also, how does this code work if what one of those pointers points to are declared volatile? What if both pointers point to volatile things? What happens if one of those pointers crosses through 0 because it rolled over from max?

Thanks. I doubt you could have provided a better example of the precise problem.



> This is the code that gives us the classic buffer overflow vulnerability all because checking for a length isn't elegant.

The few characters it would take to check length didn't effect this API, it was just part of the decision to use null terminated strings instead of storing lengths.

>Also, how does this code work if what one of those pointers points to are declared volatile? What if both pointers point to volatile things?

I can't imagine any way in which volatile would cause problems. What are you pondering? strcpy doesn't take volatile pointers anyway.

> What happens if one of those pointers crosses through 0 because it rolled over from max?

That's just a very specific type of buffer overflow. strcpy is the wrong function to use in such a scenario.

> Thanks. I doubt you could have provided a better example of the precise problem.

strcpy is for very specific situations. Writing it in a simple way isn't the problem. It's not like it's even theoretically possible for strpy to avoid overflowing. It doesn't have enough information, only two pointers.

In short, this is a bad API, it is not a bad implementation.


> I can't imagine any way in which volatile would cause problems. What are you pondering?

Termination condition. Which data applies and does it have to be reread after the assignment?


It is read once and written once, and the value that was read is the value of the expression.

Note that this "elaborated" version,

    while(*src) {
      char c = *src;
      src++;
      *dst = c;
      dst++;
    }
if the pointers were volatile, would actually read the source value twice.


The original while loop is functioning more like a do {} while() loop and the c is declared outside.

  char c;
  do {
    c = *src;
    src++;
    *dst = c;
    dst++;
  } while (c != '\0');
But, even here, people reading the code can reason about the volatiles without having to go look up the standard.

And don't even get me started about compiler implementation of volatile.


So your conclusion is that the original code is more logical with regard to volatile pointers, even though calling it with one would mean casting away the volatile and basically inviting disaster?

No coding style is immune to problems caused by changing the number of reads to a variable. Volatile is an exceptional thing that must be treated with care and heavy documentation. It has no relevance to the quality of a normal-code strcpy.


This is the code that gives us the classic buffer overflow vulnerability all because checking for a length isn't elegant.

Or rather, buffer overflows arise because someone did not think of lengths, and somehow this mentality became ingrained in a rather large number of programmers; this code is perfectly fine if the length of the source string is always less than or equal to that of the destination. I think saying "never use X" is almost always a bad idea, since it discourages reasoning, and not thinking about lengths is what lead to this problem in the first place.

On the other hand, "never use gets()" is more appropriate, since whoever came up with that function clearly never thought of lengths...




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: