Strange run time error

[sandeep]

Hello

I am reminding myself of C after many years. I have found a very strange
run time error, "Segmentation Fault", I don't remember ever seeing this.
There are no compiler errors! What is wrong with my code??

main(char * argv [], int argc)
{
char NAME[1024 * sizeof (char)] = {0}; // declare static array
// build up the output buffer
if ( strlen (argv[1]) > 1000 ) {
printf("Buffer overflow error please restart");
abort();
}
strcpy(NAME, "Hello ");
strcat(NAME, argv[1]);
strcat(NAME, " have a nice day");
// display the buffer
printf(NAME);
}

 

[Andrew Poelstra]

Hello

I am reminding myself of C after many years. I have found a very strange
run time error, "Segmentation Fault", I don't remember ever seeing this.
There are no compiler errors! What is wrong with my code??

main(char * argv [], int argc)

int main(int argc, char *argv[])

This corrects two errors - one, the implicit int return
which is no longer valid C, and two, your parameters were
out of order.

Surely your compiler warned you of these things?

{
char NAME[1024 * sizeof (char)] = {0}; // declare static array

sizeof(char) == 1 by definition. I wish they had called char 'byte'
for this reason, so that actual character data (which IRL is more
than one byte per character now) could be represented in a char,
but they didn't.

The point is that:
char name[1024] = {0};
is just as valid, and clearer too.

// build up the output buffer
if ( strlen (argv[1]) > 1000 ) {

Whoa whoa whoa, what is argv[1]? Why do you assume such a thing
to exist? You need to check first, by making sure argc is > 1.
The C Standard says that argv[argc] is NULL, and passing NULL
to strlen gets you a segfault.

(Actually, it's undefined, but most modern OS's are kind enough to
simply shoot your program in the head at the point of dereference.)

Plus your main() parameters were backwards so you're screwed
no matter what.

printf("Buffer overflow error please restart");

You're missing a \n at the end of this.

abort();

exit(EXIT_FAILURE); is more idiomatic. You need to

}
strcpy(NAME, "Hello ");
strcat(NAME, argv[1]);
strcat(NAME, " have a nice day");

You can still overflow here, because if strlen(argv[1]) is > 981,
you won't have room for the rest of the text and the NUL-ending.
You only checked the length against 1000, which is insufficient.

// display the buffer
printf(NAME);

Where is your
return 0;
?

If you're going to use implicit int, at least do it properly.

 

[Ben Bacarisse]

I am reminding myself of C after many years. I have found a very strange
run time error, "Segmentation Fault", I don't remember ever seeing
this.

It generally relates to invalid memory access.

There are no compiler errors! What is wrong with my code??

You should try asking for more warnings. You may be using Linux with
gcc. If so try

gcc -std=c99 -pedantic -Wall -Wextra

main(char * argv [], int argc)

int main(int argc, char *argv[])

You have the parameters the wrong way round.

{
char NAME[1024 * sizeof (char)] = {0}; // declare static array

This is not what most people call static array. It is, if you want to
categorise it, an automatic array.

sizeof (char) is 1 by definition and the name SHOUTS more than is
usual!

// build up the output buffer
if ( strlen (argv[1]) > 1000 ) {
printf("Buffer overflow error please restart");
abort();
}

When you fix the first problem, this may still go wrong. You need to
test that argc > 1 or you can't pass argv[1] to strlen.

strcpy(NAME, "Hello ");
strcat(NAME, argv[1]);
strcat(NAME, " have a nice day");
// display the buffer
printf(NAME);
}

To use printf, you should include <stdio.h> and <string.h> for the
others. Many compilers will get you get away with it, but this is
just them being unhelpful.

 

[Eric Sosman]

Hello

I am reminding myself of C after many years. I have found a very strange
run time error, "Segmentation Fault", I don't remember ever seeing this.
There are no compiler errors! What is wrong with my code??

Almost everything. To begin with, you're missing headers
for the library functions you call:

#include <stdio.h>
#include <stdlib.h>

main(char * argv [], int argc)

Next problem: The parameters are backwards. It doesn't
matter what you name them, but the `int' must be first and
the `char**' second.

{
char NAME[1024 * sizeof (char)] = {0}; // declare static array

Next problem: This isn't a static array. (Not really a
"problem," in the sense that you don't actually need a static
array -- but if you think you're getting one, you're wrong.)

Next problem: There's no reason to initialize the array
to zeroes. It doesn't hurt anything, but it also doesn't help:
You're going to overwrite the array anyhow -- all of it that
you care about, at any rate.

Next problem: // is a syntax error in C90. It starts a
comment in C99, but we know you're not using a C99 compiler
(because such a compiler would have complained loudly about
the type-less definition of main() and about all the calls
to undeclared library functions.)

// build up the output buffer

Same problem again: // is a syntax error.

if ( strlen (argv[1])> 1000 ) {

Next problem: Even if you swap the parameters into the
right order, you don't know whether argv[1] exists unless you
first look at argc or at argv[0]. Even if argv[1] exists, it
might be a null pointer.

printf("Buffer overflow error please restart");

Next problem: No \n character to end the line.

abort();
}
strcpy(NAME, "Hello ");
strcat(NAME, argv[1]);
strcat(NAME, " have a nice day");
// display the buffer
printf(NAME);

Next problem: If argv[1] is something like "%solution"
this printf() call will go off the rails.

Next problem: Another absent \n.

Next problem: You fall off the end of int-valued main()
without returning a value. In C99 there's a special dispensation
that makes this equivalent to returning zero, but we know you're
not using C99.

}

Final problem: All this strlen()-ing and strcat()-ing
is just plain silly. Once you've determined argv[1] exists

printf ("Hello %s have a nice day\n", argv[1]);

does the trick, safely, sleekly, and without all that foolish
mucking about.

 

[rudolf]

Next problem: Even if you swap the parameters into the
right order, you don't know whether argv[1] exists unless you
first look at argc or at argv[0]. Even if argv[1] exists, it
might be a null pointer.

How do you use argv[0] to check the presence of argv[1]?

 

[Eric Sosman]

Next problem: Even if you swap the parameters into the
right order, you don't know whether argv[1] exists unless you
first look at argc or at argv[0]. Even if argv[1] exists, it
might be a null pointer.

How do you use argv[0] to check the presence of argv[1]?

The array of command-line argument pointers is followed by
a null pointer, that is, argv[argc] == NULL. Therefore, if
argv[0] != NULL, argv[1] exists (it might be NULL, but it
exists).

 

[Seebs]

Next problem: Even if you swap the parameters into the
right order, you don't know whether argv[1] exists unless you
first look at argc or at argv[0]. Even if argv[1] exists, it
might be a null pointer.

How do you use argv[0] to check the presence of argv[1]?

If argv[0] is not NULL, then argv[1] must exist, although it may be
NULL. If argv[0] is NULL, then argv[1] does not need to even exist.

-s

 

[Richard Bos]

char NAME[1024 * sizeof (char)] = {0}; // declare static array

sizeof(char) == 1 by definition. I wish they had called char 'byte'
for this reason, so that actual character data (which IRL is more
than one byte per character now) could be represented in a char,
but they didn't.

The point is that:
char name[1024] = {0};
is just as valid, and clearer too.

The error goes deeper than that, much deeper. If sandeep had wanted 1024
ints, he would still have had to write

int sizes[1024] = {0};

and not

int sizes[1024 * sizeof (int)] = {0};

exit(EXIT_FAILURE); is more idiomatic. You need to
#include <stdlib.h>
to get that constant.

He also needs to #include it to get a declaration for exit() or abort();
not to mention <string.h> and <stdio.h>.

Richard

 

[spinoza1111]

Hello

I am reminding myself of C after many years. I have found a very strange
run time error, "Segmentation Fault", I don't remember ever seeing this.

I get 'em all the time, cowboy.

There are no compiler errors! What is wrong with my code??

main(char * argv [], int argc)

You have reversed the order in which the argument count and value are
passed to main. The count comes before the argument.

{
  char NAME[1024 * sizeof (char)] = {0}; // declare static array
  // build up the output buffer
  if ( strlen (argv[1]) > 1000 ) {

I'd suggest that it's bad practice to decide how long a string can be
and used fixed length arrays where the bounds are calculated at
compile time. Instead, use malloc.

    printf("Buffer overflow error please restart");
    abort();
  }
  strcpy(NAME, "Hello ");
  strcat(NAME, argv[1]);
  strcat(NAME, " have a nice day");
  // display the buffer
  printf(NAME);



}