Seg Fault within function (New to C)

[AMT2K5]

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

[Emmanuel Delahaye]

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 <stdio.h>
#include <string.h>

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++

[Arthur Huillet]

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.

[AMT2K5]

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.

[AMT2K5]

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.

[Old Wolf]

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.

[Eric Sosman]

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

[Keith Thompson]

Arthur Huillet <ar*****************************@free.fr> 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 <http://www.ghoti.net/~kst>
San Diego Supercomputer Center <*> <http://users.sdsc.edu/~kst>
We must do something. This is something. Therefore, we must do this.

[Emmanuel Delahaye]

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"

[Emmanuel Delahaye]

(supersedes <mn***********************@YOURBRAnoos.fr>)

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."

[CBFalconer]

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:

<http://cbfalconer.home.att.net/download/strlcpy.zip>

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

[akarl]

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 <ctype.h>
#include <stdbool.h>
#include <string.h>

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);
}

[akarl]

akarl wrote:

Seems like the source of your problem has been located...
...however, here's my version of cleanSpace which is slightly shorter:

#include <ctype.h>
#include <stdbool.h>
#include <string.h>

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 <ctype.h>
#include <stdbool.h>
#include <string.h>

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