Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > question

Reply
Thread Tools

question

 
 
amanayin
Guest
Posts: n/a
 
      09-21-2003
Why does this program run in the ddd debugger but when i try to run the
program in a konsole i get a segmentation fault. I just can't work it out
the OS i use is suse 8.2 pro program below:

/* OVERLAY.C OVERLAY OF STRINGS */
#include<malloc.h>
#include<string.h>
#include<stdio.h>
#define MAX_LEN 40
int main(void)

{
int pos;
char *line, *aster, *blank;


line = (char*)malloc(MAX_LEN);
aster = (char*)malloc(1);
blank = (char*)malloc(1);

line = " ";
aster = "*";
blank = "";

printf("Enter position of star(1 - 41): ");
scanf("%d",&pos);

if(pos > 0 && pos < 41)
{
printf("\n");
printf(" 1 2 3 4\n");
printf("1234567890123456789012345678901234567890\n ");
puts(strcat (strncat(blank,line,pos-1),aster));
}
else
printf("out of range %d\n",pos);

return 0;
}

 
Reply With Quote
 
 
 
 
Nick Austin
Guest
Posts: n/a
 
      09-21-2003
On Sun, 21 Sep 2003 15:19:55 +0000 (UTC), amanayin
<(E-Mail Removed)> wrote:

>Why does this program run in the ddd debugger but when i try to run the
>program in a konsole i get a segmentation fault. I just can't work it out
>the OS i use is suse 8.2 pro program below:
>
>/* OVERLAY.C OVERLAY OF STRINGS */
>#include<malloc.h>


Non-standard header. Should be:
#include <stdlib.h>

>#include<string.h>
>#include<stdio.h>
>#define MAX_LEN 40
>int main(void)
>
>{
> int pos;
> char *line, *aster, *blank;
>
>
> line = (char*)malloc(MAX_LEN);


You don't need the cast. Also strings need to be one byte
longer for the termination character:

line = malloc( MAX_LEN + 1 );

> aster = (char*)malloc(1);
> blank = (char*)malloc(1);
>
> line = " ";
> aster = "*";
> blank = "";


These assign new values to the pointers. Since the new
value is a string literal this means that you cannot
subsequently write to the string.

You want to copy the string literals into the newly allocated
memory using the functions from <string.h>:

strcpy( line, " " );
strcpy( aster, "*" );
strcpy( blank, "" );

> printf("Enter position of star(1 - 41): ");

fflush( stdout );
> scanf("%d",&pos);
>
> if(pos > 0 && pos < 41)
> {
> printf("\n");
> printf(" 1 2 3 4\n");
> printf("1234567890123456789012345678901234567890\n ");
> puts(strcat (strncat(blank,line,pos-1),aster));


puts() does not append a trailing newline, You'd be better
using printf instead,

And a style point. It's easier to read if each statemtent
is on it's own line:
strncat( blank, line, pos-1 );
strcat( blank, aster );
printf( "%s\n", blank );

> }
> else
> printf("out of range %d\n",pos);
>
> return 0;
>}


BTW, if you just want to print the asterix in the correct place
you can use the following:

printf( "%*s\n", pos, "*" );

Nick.

 
Reply With Quote
 
 
 
 
Irrwahn Grausewitz
Guest
Posts: n/a
 
      09-21-2003
amanayin <(E-Mail Removed)> wrote:

>Why does this program run in the ddd debugger but when i try to run the
>program in a konsole i get a segmentation fault. I just can't work it out
>the OS i use is suse 8.2 pro program below:
>
>/* OVERLAY.C OVERLAY OF STRINGS */
>#include<malloc.h>


Non-standard header, use:
#include <stdlib.h>

>#include<string.h>
>#include<stdio.h>
>#define MAX_LEN 40
>int main(void)
>
>{
> int pos;
> char *line, *aster, *blank;
>
>

Drop the casts to malloc()'s return value - they're unnecessary and
may hide the fact that no suitable prototype for malloc is in scope.


You allocate memory to hold 40 characters here ...
> line = (char*)malloc(MAX_LEN);


.... and for one character here...
> aster = (char*)malloc(1);

.... and here ...
> blank = (char*)malloc(1);
>


.... then forget your references to that memory by letting 'line',
'aster' and 'blank' point to string literals here:
> line = " ";
> aster = "*";
> blank = "";


Remember: you cannot assign strings directly in C, you have to use
strcpy() for this. But if you did, you would have invoked undefined
behaviour because you failed to allocate memory to hold the terminating
null character for 'line' and 'aster'.

>

You ask the user to enter a number up to 41 ...
> printf("Enter position of star(1 - 41): ");
> scanf("%d",&pos);
>


.... but this will print an "out of range" message if the user enters 41:
> if(pos > 0 && pos < 41)
> {
> printf("\n");
> printf(" 1 2 3 4\n");
> printf("1234567890123456789012345678901234567890\n ");


Uh-oh, now you attempt to write to those non-modifiable string
literals, invoking undefined behaviour. Luckily this resulted in
a segfault only - which indicates you are not running your programs
on a DeathStar 9000:
> puts(strcat (strncat(blank,line,pos-1),aster));
> }
> else
> printf("out of range %d\n",pos);
>
> return 0;
>}


The following program performs no string manipulation at all, yet still
produces the result (I assume) you expected:

#include <stdio.h>

int main( void )
{
int pos;

printf( "Enter position of star(1 - 40): " );
scanf( "%d", &pos );

if ( pos > 0 && pos < 41 )
{
printf( " 1 2 3 4\n" );
printf( "1234567890123456789012345678901234567890\n" );
printf( "%*s\n", pos, "*" );
}
else
printf( "out of range %d\n", pos );

return 0;
}

Regards

Irrwahn

--
do not write: void main(...)
do not use gets()
do not cast the return value of malloc()
do not fflush( stdin )
read the c.l.c-faq: http://www.eskimo.com/~scs/C-faq/top.html
 
Reply With Quote
 
Mike Wahler
Guest
Posts: n/a
 
      09-21-2003

"Nick Austin" <(E-Mail Removed)> wrote in message
news:(E-Mail Removed)...

> > puts(strcat (strncat(blank,line,pos-1),aster));

>
> puts() does not append a trailing newline,


Yes it does. (7.19.7.10)

-Mike


 
Reply With Quote
 
Al Bowers
Guest
Posts: n/a
 
      09-21-2003


amanayin wrote:
> Why does this program run in the ddd debugger but when i try to run the
> program in a konsole i get a segmentation fault. I just can't work it out
> the OS i use is suse 8.2 pro program below:


You were fortunate that the code worked on anything. There are
numerous flaws. I will point out a few but I am sure I will
miss some of the errors.

>
> /* OVERLAY.C OVERLAY OF STRINGS */
> #include<malloc.h>


replace malloc.h with stdlib.h

> #include<string.h>
> #include<stdio.h>
> #define MAX_LEN 40
> int main(void)
>
> {
> int pos;
> char *line, *aster, *blank;
>
>
> line = (char*)malloc(MAX_LEN);
> aster = (char*)malloc(1);
> blank = (char*)malloc(1);
>

Your allocations are not big enough storage for the strings
that you will put in them. Also, you should check to
make sure all the allocations were successful.

> line = " ";

Here you would want to use strcpy or memset.
> aster = "*";
> blank = "";


Use strcpy.

>
> printf("Enter position of star(1 - 41): ");

1 - 40
fflsuh(stdout);


> scanf("%d",&pos);
>
> if(pos > 0 && pos < 41)
> {
> printf("\n");
> printf(" 1 2 3 4\n");
> printf("1234567890123456789012345678901234567890\n ");
> puts(strcat (strncat(blank,line,pos-1),aster));
> }
> else
> printf("out of range %d\n",pos);
>
> return 0;
> }
>


Are you intentionally making a convoluted code for this
simple task? There are many ways to do this without the
complexity that you have chosen.

Here is your code with the recommended corrections.

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

#define MAX_LEN 41

int main(void)

{
int pos;
char *line, *aster, *blank;


line = malloc(MAX_LEN);
aster = malloc(2);
blank = malloc(MAX_LEN);

if(line == NULL || aster == NULL || blank == NULL)
{
free(line);free(aster);free(blank);
puts("Memory allocation error");
exit(EXIT_FAILURE);
}
memset(line,' ',MAX_LEN);
line[MAX_LEN] = '\0';
strcpy(aster,"*");
*blank = '\0';

printf("Enter position of star(1 - 40): ");
fflush(stdout);
scanf("%d",&pos);
if(pos > 0 && pos < 41)
{
printf("\n");
printf(" 1 2 3 4\n");
printf("1234567890123456789012345678901234567890\n ");
puts(strcat (strncat(blank,line,pos-1),aster));
}
else
printf("out of range %d\n",pos);
return 0;
}

--
Al Bowers
Tampa, Fl USA
mailto: http://www.velocityreviews.com/forums/(E-Mail Removed) (remove the x)
http://www.geocities.com/abowers822/

 
Reply With Quote
 
Irrwahn Grausewitz
Guest
Posts: n/a
 
      09-21-2003
Irrwahn Grausewitz <(E-Mail Removed)> wrote:

>on a DeathStar 9000:

DeathStation 9000
--
My other computer is a abacus.
 
Reply With Quote
 
Barry Schwarz
Guest
Posts: n/a
 
      09-21-2003
On Sun, 21 Sep 2003 15:19:55 +0000 (UTC), amanayin
<(E-Mail Removed)> wrote:

>Why does this program run in the ddd debugger but when i try to run the
>program in a konsole i get a segmentation fault. I just can't work it out
>the OS i use is suse 8.2 pro program below:


Your program produces different results during different executions
because it invokes undefined behavior (UB). UB need not be consistent
between systems. It need not even be repeatable on the same system.

As you have just discovered, when you are UNlucky, UB has the
appearance of working as designed. This leads to a false sense of
success and later a bit of shock when the UB has the good graces to
abort your program in a way that says something is really wrong.

In your case, the particular undefined behavior is attempting to
modify a string literal constant as described below.

>
>/* OVERLAY.C OVERLAY OF STRINGS */
>#include<malloc.h>


There is no malloc.h in the standard. You probably want stdlib.h
where malloc is prototyped.

>#include<string.h>
>#include<stdio.h>
>#define MAX_LEN 40
>int main(void)
>
>{
> int pos;
> char *line, *aster, *blank;
>
>
> line = (char*)malloc(MAX_LEN);


Don't cast the return from malloc if it is being assigned to a
pointer. Since malloc returns a pointer to void which is, by
definition, compatible with any other object pointer, it can never
help. On the other hand, it can cause the compiler to suppress a
warning message regarding your failure to include stdlib.h which would
cause your program to invoke UB. For the same reason discussed in
aster, you need MAX_LEN+1.

> aster = (char*)malloc(1);


Due to the mistake noted below, you don't use the three addresses you
receive from malloc. When you correct that mistake, aster will need
to point to an area at least two bytes long. This is because every
string must terminate with a '\0' and the compiler does this
automatically for quoted string literals. Thus, the string "*"
consists of the two characters '*' and '\0'.

> blank = (char*)malloc(1);


You apparently intend blank to point to the area in memory where the
result of your string manipulation will reside. You must allocate
enough space to hold the worst case result, which is 42 bytes (40
possible characters from line plus two from aster).

>
> line = " ";


This statement says "Ignore the value currently in line and replace it
with the address of the string literal." The value currently in line
points to an allocated area. line is the only variable which contains
this value. Once the value is replaced, you have no way to recover
it. This means you can never free the area you allocated. This is a
memory leak. Since you never attempt to modify the data pointed to by
line, you could delete the malloc above to eliminate this problem.

If you really want line to point to a modifiable string, then instead
of an assignment statement you should use strcpy:
strcpy(line, " ");

> aster = "*";


Ditto for aster

> blank = "";


Ditto for blank but it is worse because you do modify the data it
points to.

blank points to the string literal "". The literal is, by
definition, a non-modifiable array of char (in this case, an array of
one char whose value is '\0'). For historical reasons, the literal is
not treated as const. Nonetheless, you are not allowed to modify it.
Attempting to do so invokes UB.

Even if you were allowed to modify it, the area reserved for the
array is only one byte long. As soon as you try to concatenate
anything on to it, you invoke UB by overflowing the array.

While the strcpy approach described above will work, it is overkill.
The empty string consists of only one char and you could achieve the
same effect with a simple
*blank = '\0';

>
> printf("Enter position of star(1 - 41): ");


stdout may be buffered. Your output may not yet be visible when the
scanf executes. You can force it to be visible by adding the
statement
fflush(stdout);

> scanf("%d",&pos);
>
> if(pos > 0 && pos < 41)


To allow a value of 41, use <= or < 42.

> {
> printf("\n");
> printf(" 1 2 3 4\n");
> printf("1234567890123456789012345678901234567890\n ");
> puts(strcat (strncat(blank,line,pos-1),aster));
> }
> else
> printf("out of range %d\n",pos);
>
> return 0;
>}




<<Remove the del for email>>
 
Reply With Quote
 
Serve La
Guest
Posts: n/a
 
      09-21-2003

"Al Bowers" <(E-Mail Removed)> wrote in message
news:bkkje5$2o88h$(E-Mail Removed)-berlin.de...
> line = malloc(MAX_LEN);
> aster = malloc(2);
> blank = malloc(MAX_LEN);
>
> if(line == NULL || aster == NULL || blank == NULL)
> {
> free(line);free(aster);free(blank);
> puts("Memory allocation error");
> exit(EXIT_FAILURE);
> }
> memset(line,' ',MAX_LEN);


maybe it's me, but I really prefer
sprintf(line, "%*c", MAX_LEN, ' ');


 
Reply With Quote
 
Martin Ambuhl
Guest
Posts: n/a
 
      09-21-2003
amanayin wrote:

> Why does this program run in the ddd debugger


Why it runs with your debugger, we can't say, that being a function of that
particular program and not of C, but ...

> but when i try to run the
> program in a konsole i get a segmentation fault.


[snip]

> line = (char*)malloc(MAX_LEN);
> aster = (char*)malloc(1);
> blank = (char*)malloc(1);
>
> line = " ";
> aster = "*";
> blank = "";


Here you just lost the results of the malloc() calls, have assigned the
addresses of the literal strings to the pointers line, aster, and blank.
If you just want to point to the literal strings, don't allocate any extra
space. If you want to allocate that space and fill it:
if (!(line = malloc(MAX_LEN))) { /* handle error */ }
if (!(aster = malloc(2))) { /* handle error */ }
/* note that "*" is 2 chars, '*' and '\0'. Since your use
of blank is as a string to which you will append some number
of characters, let's assume you have a constant MAX_OUTPUT_LEN.
This is the space we want for blank. */
if (!(blank = malloc(MAX_OUTPUT_LEN))) { /* handle error */ }

memset(line, ' ' , MAX_LEN-1); /* make line all spaces except */
line[MAXLEN-1] = 0; /* for the required terminating '\0' */
strcpy(aster,"*"); /* or aster[0] = '*'; aster[1] = 0; */
/* I doubt that you really want aster to be a string. From here on,
let's assume you had a defining declaration at the top of the block
char star = '*';
*/
*blank = 0; /* or blank[0]=0; Just make blank a
zero-length string */

>
> printf("Enter position of star(1 - 41): ");
> scanf("%d",&pos);
>
> if(pos > 0 && pos < 41)


This does not agree with your instructions, where you allow 41 as an entry.

> {
> printf("\n");
> printf(" 1 2 3 4\n");
> printf("1234567890123456789012345678901234567890\n ");
> puts(strcat (strncat(blank,line,pos-1),aster));


When you set blank to point to "*", you have made it point to a string
literal, which you should not be modifying.
On the other hand, when you allocated it to have a length of one, the only
string possible for it was "", that is, the terminating '\0', so any
attempt to append to it overflows the array.
Here's an easy way to place the star in the original line (remember the
declaration of star which I suggested in the comment above):
line[pos-1] = star;
Or we could make blank have pos-1 spaces,
memset(blank, ' ', pos-1);
/* now we can follow your path with: */
blank[pos-1] = 0;
strcat(blank,aster);
/* or the better */
blank[pos-1] = star;
blank[pos] = 0;

So, in sum:
1) always allocate the space your string will need, including the final
'\0', at least until you learn to use realloc()
2) <ptr> = "<string literal>" always makes <ptr> point to tha (anonymous)
string literal. This destroys any value already in <ptr>, making
earlier calls to malloc pointless and making freeing that allocated
space impossible unless you saved a copy of the earlier value of <ptr>.
Do not try to modify string literals. The right way to copy a string
literal into space allocated is with strcpy(<ptr>,"<string literal>");
3) Almost all your calls to string functions above are pointless. When
you can accomplish what you need by assigning a char, strcpy, strncpy,
strcat are wasteful.
4) What I did not show before, and what is still missing, is freeing the
space allocated. Always free allocated space. Since the suggestions I
made before allow you to not both with aster and at least one on line
or blank, you will want to do whichever ones of the following still
hold:
free(line);
free(blank);
free(aster);



--
Martin Ambuhl

 
Reply With Quote
 
Al Bowers
Guest
Posts: n/a
 
      09-21-2003


Serve La wrote:

>> memset(line,' ',MAX_LEN);

>
>
> maybe it's me, but I really prefer
> sprintf(line, "%*c", MAX_LEN, ' ');
>


It is just a matter of preference, but if are going to use
function sprintf to pad spaces you will not need to assign
line[MAX_LEN] = '\0'
if you do this.
sprintf(line,"%*c",MAX_LEN-1,'\0');

--
Al Bowers
Tampa, Fl USA
mailto: (E-Mail Removed) (remove the x)
http://www.geocities.com/abowers822/

 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
question row filter (more of sql query question) =?Utf-8?B?YW5kcmV3MDA3?= ASP .Net 2 10-06-2005 01:07 PM
Quick Question - Newby Question =?Utf-8?B?UnlhbiBTbWl0aA==?= ASP .Net 4 02-16-2005 11:59 AM
Question on Transcender Question :-) eddiec MCSE 6 05-20-2004 06:59 AM
Question re: features of the 831 router (also a 924 question) Wayne Cisco 0 03-02-2004 07:57 PM
Syntax Question - Novice Question sean ASP .Net 1 10-20-2003 12:18 PM



Advertisments