Live data from Hacker News

Massacring C Pointers

wozniak.ca

41–50 of 300 posts

Re: Massacring C Pointers

#41
post #26

It is practically impossible to teach good programming to students that have had a prior exposure to BASIC: as potential programmers they are mentally mutilated beyond hope of regeneration. - Dijkstra, "How do we tell truths that might hurt?" (1975) The author, Traister, has came from BASIC. We have the explanation.

Well that is a dumb and inaccurate statement if ever I've read one. I mean I'm sure there might have been some nugget of truth back in the 70s in that it might have been frustrating to teach C or FORTRAN to BASIC programmers but it has as much relevance these days as any of the other memes people like to quite from yesteryear. I certainly managed the transition from BASIC to Pascal with ease and definitely follow goo…

I teach programming, and I find people rarely have any special trouble in switching to a different style or paradigm. If I was teaching in the 80's and wanted to transition someone from old school BASIC to something like C or Pascal with structured programming and blocks, I'd show them an "if" with a code block and how it's different than a conditional GOTO, yet similar in a way. Gradually the mental model evolves to incorporate the new features, and when they look back, they can understand both ways, even though on paper it's "two different paradigms."

Similarly, if I'm teaching JavaScript and want to show the student functional programming, I'll show them how a "for" loop could be converted to use map or filter, or recursion. Far from "corrupting their mind" so they can't understand other ways, the imperative style serves as a foundation to learn other styles. The paradigm shift occurs gradually on some subconscious level, with the previous paradigm serving as a mental model to build up the new one.

Re: Massacring C Pointers

#42
post #38
post #27

Earlier quoted context omitted.

Most of these points are covered by the other comments. As a C programmer professionally, I'll go into a little more depth, and offer an alternative implementation for comparison. The function in question: char *combine(s, t) char *s, *t; { int x, y; char r[100]; strcpy(r, s); y = strlen(r); for (x = y; *t != '\0'; ++x) r[x] = *t++; r[x] = '\0'; return(r); } 1. The array 'r' is allocated on the stack, and returned fr…

You can avoid the need for setting the null terminator explicitly by using memcpy(str + slen, t, tlen + 1) as the second memcpy.

And this is why we have code review ;)

Re: Massacring C Pointers

#43
post #24

Earlier quoted context omitted.

It is wrong in many ways. It copies s and then t to a fixed size buffer, without any checks. That will write to invalid memory (probably smashing the stack) if len(s), len(t) or len(s) + len(t) > 100. It returns a stack allocated buffer (r) pointer to the caller. The array will be invalid when the function returns, as the automatic variables only live in the function scope (during the call), they are deallocated when…

I'd suggest not insulting the people reading your comment if you want some points back…

You are right. Fixed.

Re: Massacring C Pointers

#44

Yesterday I encountered a similar program on a HN comment chain as shown in this link. I am genuinely confused as to why this program is bad. I am a student and I do not know the best practices regarding pointers, but it is how I would write a program to combine two strings. Can someone please elaborate why it is bad? Are their any good resources to fill gaps in my knowledge? Thanks in advance. Edit: Thank you guys f…

Putting my CR hat on:

   1 char *combine(s, t)
   2 char *s, *t;
   3 {
   4
   5   int x, y;
   6   char r[100];
   7
   8   strcpy(r, s);
   9   y = strlen(r);
  10   for (x = y; *t != '\0'; ++x)
  11     r[x] = *t++;
  12 
  13    r[x] = '\0';
  14
  15    return(r);
  16
  17 }
There are several critical memory errors here.

1) The function is returning the address of a local variable. This alone makes this function rubbish.

2) The pointers s and t are unknown length, we have no guarantees that concatenating them will fit in a 100 character array. Also, he probably should be using malloc to dynamically allocate the space.

3) Use of the function strcpy rather than strncpy. He should have measured the length of s first, and then use strncpy if s was longer than 99 characters (don't forget the null terminator!). Then, rather than using a for loop, call strncpy again to copy the rest into a safe buffer size (the for loop is rather silly). The reason for this is that strcpy will cheerfully start copying past the array 'boundaries', and in this case, since he's copying into a local variable on the stack, is setting himself up for a Remote Code Execution attack if this ever gets untrusted input.

So those are the critical errors. These tie directly into why Geoff argues that the author doesn't understand the stack.

So let's, for educational purposes, go into this. We're going to go a bit into the weeds here. Sorry about that. This would be easier with a whiteboard. :)

When you fire up a program, the programs machine instructions get copied into memory, let's pretend at the memory location 0x1000. Far away from that code, at the highest memory values (more complicated on modern virtual systems, but hey, let's go back in time here :) ), the computer keeps track of a location called the stack pointer.

I'm going to put forward 3 diagrams now. Please forgive any off by one errors.

  (Diagram a)
    Registers
    A 0
    B 0
    C 0
    SP 0xffff
    PC 0x1000

    Address  Mnemonic   DATA
  PC0x1000   MOV 1, A   0x00 0x01 0x01 0x01
    0x1004   MOV A, C   0x00 0x01 0x03 0x01
    0x1008   PUSH 3     0x01 0x00 0x00 0x03
    ...      ...
    ...      ...
    ...      ...
    0xfffc   XXXXXXXX   0x00 0x00 0x00 0x00 
The program starts at 0x1000, then after executing the first two move (MOV) instructions, the state of the world becomes as follows

  (Diagram b)
    Registers
    A 1
    B 0
    C 1
    SP 0xfffe
    PC 0x1000

    Address  Mnemonic   DATA
    0x1000   MOV 1, A   0x00 0x01 0x01 0x01
    0x1004   MOV A, C   0x00 0x01 0x03 0x01
  PC0x1008   PUSH 3     0x01 0x00 0x00 0x03
    ...      ...
    ...      ...
    ...      ...                     v------\
    0xfffc   XXXXXXXX   0x00 0x00 0x00 0x03 ^-- Stack Pointer is here
When you have code like

  void function() {
     int a = 5;
     int b = 2;
     return a;
  }

  void main() {
    return function();
  }
It'll get turned into something like (I've set a 'break point' at 0x2010)

  (Diagram c)
    Registers
    A 5
    B 0
    C 0
    SP 0xfff8
    PC 0x2010

    Address  Mnemonic      DATA
    # Main starts here
    # (Note, in C, there is actually code that gets executed before this)
  PC0x1000   PUSH 0x1008   # We want to remember where to return to, so we push it to the stack.
    0x1004   JMP  0x2000
    0x1008   EXIT A        # In this implementation of C, the A register will propagate return values
    ...      ... 
    # Function 'function' is here
    0x2000   PUSH 5        # Local variables go on the stack.
    0x2004   PUSH 2
    0x2008   MOV [SP+2], A # Locally, we refer to local variables by
                           # offsets to the stack pointer, so if this function
                           # were to call itself, the stack would keep growing down
                           # but these values would be good.
    0x200c   MOV SP+2, SP  # Reset the stack before returning
  PC0x2010   JMP #SP       # Made up notation. Look at the value of the stack pointer, pop it, and jump to it.
                           # in x86, this is kinda what RET does.
    ...      ...
    ...      ...           ...
    ...      ...           ...         
    0xfff8   XXXXXXXX      0x00 0x00 0x00 0x00SP  
    0xfffc   XXXXXXXX      0x02 0x05 0x10 0x08
Okay! So with the above diagrams in mind, let's recap what goes on the stack. Local variables and return addresses. Each time a function gets called it moves the stack pointer down[1] (to lower memory addresses) to make room for local variables. So after you return from that function, and then call another function (or heck, the same one) that pointer you have that was supposed to be the concatenated string is now going to have it's values overwritten.

Furthermore, if the input strings are longer than expected, than they can overwrite values on the stack itself, including the return addres, causing your program to jump to some (if you're lucky) random location in memory.

Honestly, some of the best ways to get intuition for how the stack works, and the things that can go wrong, are CTFS at overthewire.org.

Also, https://microcorruption.com/

http://overthewire.org/wargames/bandit/

http://overthewire.org/wargames/leviathan/

[1] Sorry, 'down' means lower memory addresses, even though the displays of memory layouts always have lower memory addresses "up". :(

Re: Massacring C Pointers

#45

Earlier quoted context omitted.

That's something C's type system doesn't check for. If you want protection for this case, use C++ or any other higher-level language instead.

Well to be pedantic C++'s type system doesn't check for that either it just passes around a size_t and char *.

I was referring to std::string, which is what you should be using if you're handling textual data natively.

Re: Massacring C Pointers

#46
post #17

Yesterday I encountered a similar program on a HN comment chain as shown in this link. I am genuinely confused as to why this program is bad. I am a student and I do not know the best practices regarding pointers, but it is how I would write a program to combine two strings. Can someone please elaborate why it is bad? Are their any good resources to fill gaps in my knowledge? Thanks in advance. Edit: Thank you guys f…

I haven't touched C in years, but here's my descending "wtf" list: 1. Returns pointer to stack-allocated data, which immediately becomes invalid. Instead, it should be using some sort of allocation (e.g. 'malloc'), or taking in a destination pointer. 2. 'r' is arbitrarily set with length 100. Smaller strings don't need all that space, and larger strings definitely will overrun. 3. The function signature is really awk…

> 'strcpy' should usually be replaced by 'strncpy'

Sorry to butt in, but this is a bit of a trigger for me: I’ve had to fix a number of programs infected with this idea.

The main problems with strncpy are:

When the source string is shorter than n, strncpy will pad the target to n bytes, filling with zeros. This is bad for performance.

When the source string is longer than n, strncpy will copy n bytes but _not_ nul-terminate the target. So you need extra schenanigans every time you use it to cover this case.

So strncpy is hardly ever a good idea. Sadly there is no standard replacement that is widely accepted. More details at https://en.wikipedia.org/wiki/C_string_handling#Replacements

Re: Massacring C Pointers

#47
post #22

Earlier quoted context omitted.

strlen is not safe and the design of this function does not permit it to ever be safe. That is my point. Most "nice" C functions are also not used as teaching examples. There's a difference in how one writes C code for production use and instructive use.

I take it you come from a higher level language, where null termination would seem risky. In C, however, strlen is considered safe (as opposed to say strcpy, strcat, etc. which do have "safe" replacements). As for terse examples being given to beginners, here's the example The C Programming Language gives for strcpy: void strcpy(char *s, char *t) { while ((*s++ = *t++) != '\0') ; }

C11 introduced strnlen_s, a "safe" replacement to strlen.

Re: Massacring C Pointers

#48
post #17

Earlier quoted context omitted.

I haven't touched C in years, but here's my descending "wtf" list: 1. Returns pointer to stack-allocated data, which immediately becomes invalid. Instead, it should be using some sort of allocation (e.g. 'malloc'), or taking in a destination pointer. 2. 'r' is arbitrarily set with length 100. Smaller strings don't need all that space, and larger strings definitely will overrun. 3. The function signature is really awk…

> 'strcpy' should usually be replaced by 'strncpy' Sorry to butt in, but this is a bit of a trigger for me: I’ve had to fix a number of programs infected with this idea. The main problems with strncpy are: When the source string is shorter than n, strncpy will pad the target to n bytes, filling with zeros. This is bad for performance. When the source string is longer than n, strncpy will copy n bytes but _not_ nul-te…

There's strlcpy, but it's not part of POSIX unfortunately.

Re: Massacring C Pointers

#49
post #26

It is practically impossible to teach good programming to students that have had a prior exposure to BASIC: as potential programmers they are mentally mutilated beyond hope of regeneration. - Dijkstra, "How do we tell truths that might hurt?" (1975) The author, Traister, has came from BASIC. We have the explanation.

Well that is a dumb and inaccurate statement if ever I've read one. I mean I'm sure there might have been some nugget of truth back in the 70s in that it might have been frustrating to teach C or FORTRAN to BASIC programmers but it has as much relevance these days as any of the other memes people like to quite from yesteryear. I certainly managed the transition from BASIC to Pascal with ease and definitely follow goo…

Well, we're talking about the early nineties.

Contemporary VBA developers (or whatever it is about) can point their pitchforks vertically, ok?

Re: Massacring C Pointers

#50
post #11

Yesterday I encountered a similar program on a HN comment chain as shown in this link. I am genuinely confused as to why this program is bad. I am a student and I do not know the best practices regarding pointers, but it is how I would write a program to combine two strings. Can someone please elaborate why it is bad? Are their any good resources to fill gaps in my knowledge? Thanks in advance. Edit: Thank you guys f…

There are a few bugs in it. It returns something on the local functions stack (r) which gets torn down after return. It uses the insecure strcpy allowing the caller to stack overflow the destination. It increments t in the loop after dereferencing it which only changes the value pointed to by t. x and y are improperly initialized, it null terminates r without checking for x's value(again a buffer overflow) and there…

> It increments t in the loop after dereferencing it which only changes the value pointed to by t.

This is wrong. Postfix increment has a higher precedence than the dereference operator.

Post reply on HN