Strange run time error
sandeep
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
#include <stdlib.h>
to get that constant.
> }
> 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.
> }
--
Andrew Poelstra
http://www.wpsoftware.net/andrew
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.
--
Ben.
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>
#include <string.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.
--
Eric Sosman
eso...@ieee-dot-org.invalid
rudolf
Eric Sosman <eso...@ieee-dot-org.invalid> wrote:
> 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
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).
--
Eric Sosman
eso...@ieee-dot-org.invalid
Seebs
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
--
Copyright 2010, all wrongs reversed. Peter Seebach / usenet...@seebs.net
http://www.seebs.net/log/ <-- lawsuits, religion, and funny pictures
http://en.wikipedia.org/wiki/Fair_Game_(Scientology) <-- get educated!
Richard Bos
> On 2010-03-29, sandeep <nos...@nospam.com> wrote:
> > 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};
> > abort();
>
> 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.
Richard Heathfield
> On Mar 30, 1:30 am, sandeep <nos...@nospam.com> wrote:
>> 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.
Why am I not surprised?
>
>> 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.
No, he hasn't. The argument count is still passed to main as the first
parameter of the function, and the pointer to the first in an array of
command line arguments is still passed to main as the second parameter
of the function. His error does not change this fact. It would be truer
to say that he has got char *argv[] and argc the wrong way round in his
parameter list.
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.
Even a stopped clock is right twice a day...
>
>
>> 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);
...but you missed a great opportunity to match the stopped clock's
performance. If argv[1] (after we've fixed up the parameter ordering)
points to a string that contains, say, %s or %d, then the behaviour of
this code is undefined. He should use printf("%s\n", NAME); instead.
--
Richard Heathfield <http://www.cpax.org.uk>
Email: -http://www. +rjh@
"Usenet is a strange place" - dmr 29 July 1999
Sig line vacant - apply within