432,275 Members | 920 Online
Need help? Post your question and get tips & solutions from a community of 432,275 IT Pros & Developers. It's quick & easy.

# Seg Fault within function (New to C)

 P: n/a Hello. I have the following function titled cleanSpace that recieves a string and cleans it up. My program passes a string to the function via "cleanSpace(c)". The function works the first time through, but the second time through I recieve a seg fault [core dump] on the second last line, strcpy. I am certain something is wrong with the pointer and the way I use it. Could somebody identify what I have done wrong at that line? int cleanSpace(char* ptr) { char temp[300]; strcpy(temp, ptr); int i=0, j=0, k=0, m=0; /* Ultimately this loop will scan for new lines and tabs and replace them with spaces. */ for(i=0; temp[i]; i++) { if(temp[i] == '\n' || temp[i] == '\t') temp[i] = ' '; } // For loop finds character starting point. for(i=0; temp[i] == ' '; i++) { temp[m] = temp[i+1]; } // For loop moves all characters next to the first found character. for(i++; temp[i]; i++) { temp[++m] = temp[i]; } temp[m+1] = '\0'; // For loop removes trailing spaces. for(i = strlen(temp) - 1; temp[i] == ' '; i--) { temp[i] = '\0'; } // For loop removes excess spaces. for(i = 0; temp[i]; i++) { if(temp[i] == ' ' && temp[i+1] == ' ') { j = i; while(temp[j] == ' ') { j++; } for(k = i + 1; temp[k]; k++, j++) { temp[k] = temp[j]; } j=0; } } strcpy(ptr,temp); // Copy temp to ptr return strlen(temp); // Return the length } Appreciate any feedback - Aaron T Nov 15 '05 #1
12 Replies

 P: n/a AMT2K5 wrote on 17/07/05 : Hello. I have the following function titled cleanSpace that recieves a string and cleans it up. My program passes a string to the function via "cleanSpace(c)". The function works the first time through, but the second time through I recieve a seg fault [core dump] on the second last line, strcpy. The code is correct, but these copies are useless. Be sure that the original string is mutable (A string literal is not). Also, try to reduce the variables scope. It prepares to modularization... This works : #include #include int cleanSpace (char *ptr) { int m = 0; /* Ultimately this loop will scan for new lines and tabs and replace * them with spaces. */ { int i; for (i = 0; ptr[i]; i++) { if (ptr[i] == '\n' || ptr[i] == '\t') { ptr[i] = ' '; } } } /* For loop finds character starting point. */ { int i; for (i = 0; ptr[i] == ' '; i++) { ptr[m] = ptr[i + 1]; } /* For loop moves all characters next to the first found character. */ for (i++; ptr[i]; i++) { ptr[++m] = ptr[i]; } ptr[m + 1] = '\0'; } /* For loop removes trailing spaces. */ { int i; for (i = strlen (ptr) - 1; ptr[i] == ' '; i--) { ptr[i] = '\0'; } } /* For loop removes excess spaces. */ { int i; for (i = 0; ptr[i]; i++) { int j; if (ptr[i] == ' ' && ptr[i + 1] == ' ') { j = i; while (ptr[j] == ' ') { j++; } { int k = 0; for (k = i + 1; ptr[k]; k++, j++) { ptr[k] = ptr[j]; } j = 0; } } } } /* Return the length */ return strlen (ptr); } int main (void) { char s[] = " Clean space test string "; int n = cleanSpace (s); printf ("n=%d s='%s'\n", n, s); return 0; } -- Emmanuel The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html The C-library: http://www.dinkumware.com/refxc.html "Clearly your code does not meet the original spec." "You are sentenced to 30 lashes with a wet noodle." -- Jerry Coffin in a.l.c.c++ Nov 15 '05 #2

 P: n/a Hi, On Sun, 17 Jul 2005 10:02:06 -0700, AMT2K5 wrote: strcpy(temp, ptr); This is useless as Emmanuel Delahaye already told you, besides, strcpy should never be used. As a good security practice, you should always be using strncpy which will prevent silly crashes and potential security flaws. -- Greetings, A.H. Nov 15 '05 #3

 P: n/a Thanks for the help Arthur and Emannuel. Is there a condition I can place in the function to test if the incomming string does not need changing or is a constant string literal? I think this is also why my program causes a seg fault with the above code as well. It's acting on the string when it should not be. Nov 15 '05 #4

 P: n/a Thanks for the help Arthur and Emannuel. Is there a condition I can place in the function to test if the incomming string does not need changing or is a constant string literal? I think this is also why my program causes a seg fault with the above code as well. It's acting on the string when it should not be. Nov 15 '05 #5

 P: n/a AMT2K5 wrote: I am certain something is wrong with the pointer and the way I use it. Could somebody identify what I have done wrong at that line? // For loop removes trailing spaces. for(i = strlen(temp) - 1; temp[i] == ' '; i--) { temp[i] = '\0'; } If the string consisted entirely of spaces at this point, then you would wind off the front of the string. (This isn't actually possible because you removed leading spaces earlier, but it would be good to put a comment in the code anyway, in case you make some modification at a later date that activates the possibility of the string being entirely spaces at this point). strcpy(ptr,temp); // Copy temp to ptr That won't work if 'ptr' is not writable (eg. if you write cleanSpace(" hello "); Someone else suggested using strncpy instead of strcpy, but I disagree: strncpy is difficult to use correctly, and your function can only ever remove characters, the string cannot get longer. You cannot test within your function whether a pointer is safe to write to, or not. A good way to avoid this issue is to use 'const' in your parameters. If you have: size_t cleanSpace(char *c); that tells the reader "*c might be modified", and the reader should be sure to only call it with modifiable data. But if you write: size_t cleanSpace(char const *c); it tells the reader that the characters cannot be modified, so it is safe to write cleanSpace(" hello "), for example. If you are using GCC you can use the "-Wwritable-strings" flag which will cause a compile error if you try and write cleanSpace(" hello ") with your version of the function. Nov 15 '05 #6

 P: n/a Arthur Huillet wrote: [...] besides, strcpy should never be used. As a good security practice, you should always be using strncpy which will prevent silly crashes and potential security flaws. strcpy() is like Ubik: Safe when used as directed. strncpy() is not a "safe strcpy()." Consider char buff[13]; strncpy (buff, "Hello, world!", sizeof buff); What's "safe" about the state of affairs at this point? It's a crash waiting to happen. Since strcpy() and strncpy() require similar amounts of care for safe use, and since the latter can be hugely inefficient (replace `13' by `32768' in the example above), I can think of no good reason[*] to use strncpy(). [*] Not any more, that is. Once upon a time there was an operating system that stored a file name in a fixed-length array, either filled completely or zero-padded. (Note that such arrays are "strings" only if *not* filled completely.) strncpy() was useful on that O/S and for that one purpose -- but the O/S has long since been superseded and improved, and the purpose has, as far as I can see, vanished. -- Eric Sosman es*****@acm-dot-org.invalid Nov 15 '05 #7

 P: n/a Arthur Huillet writes: On Sun, 17 Jul 2005 10:02:06 -0700, AMT2K5 wrote: strcpy(temp, ptr); This is useless as Emmanuel Delahaye already told you, besides, strcpy should never be used. As a good security practice, you should always be using strncpy which will prevent silly crashes and potential security flaws. I disagree. strcpy() is safe if (and only if!) you're sure that the target has enough room to hold the source string. strncpy(), on the other hand, can sometimes create a result that isn't terminated with a '\0' character (i.e., isn't a valid string). Some people here have advocated a non-standard function called strlcpy(), which is included in OpenBSD and is available as opens source. I don't know enough about it to comment on it, except that strictly speaking the name infringes on the implementation's namespace, but that's unlikely to be a problem in practice. -- Keith Thompson (The_Other_Keith) ks***@mib.org San Diego Supercomputer Center <*> We must do something. This is something. Therefore, we must do this. Nov 15 '05 #8

 P: n/a AMT2K5 wrote on 17/07/05 : Is there a condition I can place in the function to test if the incomming string does not need changing or is a constant string literal? Not to my knowledge. The person who write the application code must be informed of the restrictions. One clue : the absence of 'const' in the parameter definition. -- Emmanuel The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html The C-library: http://www.dinkumware.com/refxc.html "C is a sharp tool" Nov 15 '05 #9

 P: n/a (supersedes ) AMT2K5 wrote on 17/07/05 : Is there a condition I can place in the function to test if the incomming string does not need changing or is a constant string literal? Not to my knowledge. The person who writes the application code must be informed of the restrictions. One clue : the absence of 'const' in the parameter definition. -- Emmanuel The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html The C-library: http://www.dinkumware.com/refxc.html "There are 10 types of people in the world today; those that understand binary, and those that dont." Nov 15 '05 #10

 P: n/a Eric Sosman wrote: Arthur Huillet wrote: [...] besides, strcpy should never be used. As a good security practice, you should always be using strncpy which will prevent silly crashes and potential security flaws. strcpy() is like Ubik: Safe when used as directed. strncpy() is not a "safe strcpy()." Consider char buff[13]; strncpy (buff, "Hello, world!", sizeof buff); What's "safe" about the state of affairs at this point? It's a crash waiting to happen. Since strcpy() and strncpy() require similar amounts of care for safe use, and since the latter can be hugely inefficient (replace `13' by `32768' in the example above), I can think of no good reason[*] to use strncpy(). The solution is simple. Use strlcpy and strlcat. If your library does not already have them (BSD?) you can get the completely portable source at: which uses the theoretically illegal names strlcat and strlcpy. However, having the source, you can easily change them if they create difficulties. -- "If you want to post a followup via groups.google.com, don't use the broken "Reply" link at the bottom of the article. Click on "show options" at the top of the article, then click on the "Reply" at the bottom of the article headers." - Keith Thompson Nov 15 '05 #11

 P: n/a AMT2K5 wrote: Hello. I have the following function titled cleanSpace that recieves a string and cleans it up. My program passes a string to the function via "cleanSpace(c)". The function works the first time through, but the second time through I recieve a seg fault [core dump] on the second last line, strcpy. I am certain something is wrong with the pointer and the way I use it. Could somebody identify what I have done wrong at that line? int cleanSpace(char* ptr) { char temp[300]; strcpy(temp, ptr); int i=0, j=0, k=0, m=0; /* Ultimately this loop will scan for new lines and tabs and replace them with spaces. */ for(i=0; temp[i]; i++) { if(temp[i] == '\n' || temp[i] == '\t') temp[i] = ' '; } // For loop finds character starting point. for(i=0; temp[i] == ' '; i++) { temp[m] = temp[i+1]; } // For loop moves all characters next to the first found character. for(i++; temp[i]; i++) { temp[++m] = temp[i]; } temp[m+1] = '\0'; // For loop removes trailing spaces. for(i = strlen(temp) - 1; temp[i] == ' '; i--) { temp[i] = '\0'; } // For loop removes excess spaces. for(i = 0; temp[i]; i++) { if(temp[i] == ' ' && temp[i+1] == ' ') { j = i; while(temp[j] == ' ') { j++; } for(k = i + 1; temp[k]; k++, j++) { temp[k] = temp[j]; } j=0; } } strcpy(ptr,temp); // Copy temp to ptr return strlen(temp); // Return the length } Seems like the source of your problem has been located... ....however, here's my version of cleanSpace which is slightly shorter: #include #include #include int cleanSpace(char *s) { int lower = 0, upper = 0; while (true) { while ((s[upper] != '\0') && isspace(s[upper])) { upper++; } while ((s[upper] != '\0') && !isspace(s[upper])) { s[lower] = s[upper]; lower++; upper++; } if (s[upper] == '\0') { break; } s[lower] = ' '; lower++; } if ((lower == 0) || ((lower > 0) && (s[lower - 1] != ' '))) { s[lower] = '\0'; } else { s[lower - 1] = '\0'; } return strlen(s); } Nov 15 '05 #12

 P: n/a akarl wrote: Seems like the source of your problem has been located... ...however, here's my version of cleanSpace which is slightly shorter: #include #include #include int cleanSpace(char *s) { int lower = 0, upper = 0; while (true) { while ((s[upper] != '\0') && isspace(s[upper])) { upper++; } while ((s[upper] != '\0') && !isspace(s[upper])) { s[lower] = s[upper]; lower++; upper++; } if (s[upper] == '\0') { break; } s[lower] = ' '; lower++; } if ((lower == 0) || ((lower > 0) && (s[lower - 1] != ' '))) { s[lower] = '\0'; } else { s[lower - 1] = '\0'; } return strlen(s); } ....and here is a slightly improved version: #include #include #include size_t cleanSpace(char *s) { int lower = 0, upper = 0; while (true) { while (isspace(s[upper])) { upper++; } while (!isspace(s[upper]) && (s[upper] != '\0')) { s[lower] = s[upper]; lower++; upper++; } if (s[upper] == '\0') { break; } s[lower] = ' '; lower++; } if ((lower > 0) && (s[lower - 1] == ' ')) { s[lower - 1] = '\0'; } else { s[lower] = '\0'; } return strlen(s); } August Nov 15 '05 #13

### This discussion thread is closed

Replies have been disabled for this discussion.