Bus error--not sure why

[Kevin Walzer]

The following code compiles with gcc but returns a bus error when I run
the executable. Any ideas?

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


/* generate serial number list*/
int main (void) {

char appname[] = "MYAPP"; //new app
char myserial[100]; //pointer variable for serial numbers
int count = 20000; //number of serial numbers
int i; //index number;
char *serials[count+1]; //full list of serial numbers
int seriallength; //length of serial number
char *serialblock; //block of memory for serial number

//trim application name
char *shortname = (char *) malloc(2);
strncpy(shortname, appname, 2);
free(shortname);

//build the serial number array
for (i = 1; i < count; ++i) {
int five = i*5; //first number of the serial array
int eleven = i/11; //second number
int one = (i-1); //third number
sprintf(myserial, "%s-%i-%i-%i-%s", appname, five, eleven, one,
shortname);//assign component values to serial number string
seriallength=strlen(myserial); //length of serial number
serialblock = (char *) malloc(seriallength + 1); //allocate memory
for serial
if (serialblock == NULL) {
return 0; //check for null memory
}

strcpy(serialblock, myserial); //copy data to address of serial
block from serial
serials=serialblock; //assign data at serial block address to
next indeas of serials array

free(serialblock); //free memory
}

for (i = 1; i < count; ++i) {
printf(" %s\n", i, serials);
}

return 0;



}

 

[Jens Thoms Toerring]

The following code compiles with gcc but returns a bus error when I run
the executable. Any ideas?

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

/* generate serial number list*/
int main (void) {

char appname[] = "MYAPP"; //new app
char myserial[100]; //pointer variable for serial numbers
int count = 20000; //number of serial numbers
int i; //index number;
char *serials[count+1]; //full list of serial numbers

Why 'count+1' when your lop only runs from 1 to 'count-1'?

int seriallength; //length of serial number
char *serialblock; //block of memory for serial number

//trim application name
char *shortname = (char *) malloc(2);
strncpy(shortname, appname, 2);
free(shortname);

The last three lines are a NOP. You allocate memory, copy
something to it and deallocate it (and casting the return
value of malloc() doesn't make much sense).

//build the serial number array
for (i = 1; i < count; ++i) {
int five = i*5; //first number of the serial array
int eleven = i/11; //second number
int one = (i-1); //third number
sprintf(myserial, "%s-%i-%i-%i-%s", appname, five, eleven, one,
shortname);//assign component values to serial number string

You can't use 'shortname' anymore, you already deallocated what
it was pointing to. And even if you hadn't it would not be a
string since there is no trailing '\0' character (please read
the description of strncpy() again and carefully note what hap-
pens if the source string is longer than what you allow to be
copied). So even then using "%s" isn't an option since it re-
quires a string. If you wouldn't have deallocated the memory
you would have been able to print it out using "%.2s" (to keep
printf() from using more than the first two chars). But then
you could get the same effect by using "%.2s" with 'appname',
so the whole use of 'shortname' seems to be rather useless.

seriallength=strlen(myserial); //length of serial number
serialblock = (char *) malloc(seriallength + 1); //allocate memory
for serial
if (serialblock == NULL) {
return 0; //check for null memory
}

strcpy(serialblock, myserial); //copy data to address of serial
block from serial
serials=serialblock; //assign data at serial block address to
next indeas of serials array

free(serialblock); //free memory

And now 'serials' points to free-ed memory you aren't allowed
to use for anything at all. You seem to assume that just holding
a pointer to something would keep what it points to in existence,
but that's not the case. (Perhaps you're too much used to a lan-
guage with a GC?) One way to go would be to not deallocate
'serialblock' but just keep the pointer stored in the 'serials'
array and deallocate the memory only when you don't need it
anymore (e.g. after the printf() later on).

}

for (i = 1; i < count; ++i) {
printf(" %s\n", i, serials);

And here you use pointers to memory that already has been deallocated
again and again.

}

return 0;

Are you sure you're getting a bus error (I guess that means a SIGBUS)
instead of a segmentation fault (SIGSEGV)?

Regards, Jens

 

[Barry Schwarz]

The following code compiles with gcc but returns a bus error when I run
the executable. Any ideas?

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


/* generate serial number list*/
int main (void) {

char appname[] = "MYAPP"; //new app
char myserial[100]; //pointer variable for serial numbers
int count = 20000; //number of serial numbers
int i; //index number;
char *serials[count+1]; //full list of serial numbers
int seriallength; //length of serial number
char *serialblock; //block of memory for serial number

//trim application name
char *shortname = (char *) malloc(2);

Don't cast the return from malloc. It doesn't help anything and can
suppress a diagnostic message you would really want to see.

strncpy(shortname, appname, 2);

This invokes undefined behavior by writing beyond you allocated
memory. appname contains 6 char. You allocated space for 2. The
adage about 10 pounds in a 5 pound bag applies.

free(shortname);

On many installations, writing beyond allocated memory messes up
internal allocation control tables and free gets very confused.

What is the point of these three statements? You allocate memory,
copy into it, and then free it without accomplishing anything.

//build the serial number array
for (i = 1; i < count; ++i) {
int five = i*5; //first number of the serial array
int eleven = i/11; //second number
int one = (i-1); //third number
sprintf(myserial, "%s-%i-%i-%i-%s", appname, five, eleven, one,
shortname);//assign component values to serial number string

More undefined behavior. You freed shortname. You cannot attempt to
access the memory it used to point to. In fact, you cannot evaluate
the value it contains for any purpose.

seriallength=strlen(myserial); //length of serial number
serialblock = (char *) malloc(seriallength + 1); //allocate memory
for serial
if (serialblock == NULL) {
return 0; //check for null memory

It is a little strange to return success when your program failed.

}

strcpy(serialblock, myserial); //copy data to address of serial
block from serial

This is why you should not use // comments in a usenet post.

serials=serialblock; //assign data at serial block address to
next indeas of serials array

free(serialblock); //free memory

This releases the memory that serial points to.

}

for (i = 1; i < count; ++i) {
printf(" %s\n", i, serials);

And this invokes undefined behavior because you no longer own the
memory serial used to point to.

}

return 0;



}

Remove del for email

 

[Kevin Walzer]

Kevin Walzer <[email protected]> wrote:

The last three lines are a NOP. You allocate memory, copy
something to it and deallocate it (and casting the return
value of malloc() doesn't make much sense).

You can't use 'shortname' anymore, you already deallocated what
it was pointing to. And even if you hadn't it would not be a
string since there is no trailing '\0' character (please read
the description of strncpy() again and carefully note what hap-
pens if the source string is longer than what you allow to be
copied). So even then using "%s" isn't an option since it re-
quires a string. If you wouldn't have deallocated the memory
you would have been able to print it out using "%.2s" (to keep
printf() from using more than the first two chars). But then
you could get the same effect by using "%.2s" with 'appname',
so the whole use of 'shortname' seems to be rather useless.

And now 'serials' points to free-ed memory you aren't allowed
to use for anything at all. You seem to assume that just holding
a pointer to something would keep what it points to in existence,
but that's not the case. (Perhaps you're too much used to a lan-
guage with a GC?) One way to go would be to not deallocate
'serialblock' but just keep the pointer stored in the 'serials'
array and deallocate the memory only when you don't need it
anymore (e.g. after the printf() later on).

And here you use pointers to memory that already has been deallocated
again and again.

Thank you for your detailed response to my code. It was very helpful and
enabled me to solve the problems I was encountering.

Yes, I am still learning the rules for memory management. I have a lot
of experience with dynamic/scripting languages (Tcl and Python) but very
little with C.

For the record, here is the code I came up with, which compiles and runs
without error:

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


/* regproc.c--generate serial number list*/

int main (void) {

/* new app */
char appname[] = "MYAPP";

/* pointer variable for serial numbers */
char myserial[100];

/* number of serial numbers */
int count = 20000;

/* index number */
int i;

/* full array of serial numbers */
char *serials[count];

/* length of individual serial number */
int seriallength;

/* placeholder for individual serial number in serials array */
char *serialnumber;

/* character buffer for abbreviated app name: first two letters plus \0
character */
char shortname[3];

/* now trim the application name to the first two letters, copy to
"shortname" array, append to the serial number later */
strncpy(shortname, appname, 2);

/* now build the serial number array */
for (i = 1; i < count; ++i) {
/* second item of the array; first is the app name */
int five = i*5;
/* third item of the array */
int eleven = i/11;
/* fourth item of the array */
int one = (i-1);
/* dump all serial number components to "myserial" character
array--appname, five, eleven, one, shortname */
sprintf(myserial, "%s-%i-%i-%i-%s", appname, five, eleven, one,
shortname);
/* get length of "myserial" string */
seriallength=strlen(myserial);

/* allocate memory for individual serial numbers in array, check for
null */
serialnumber = (char *) malloc(seriallength);
if (serialnumber == NULL) {
return 0; //check for null memory
}

/* copy value of "myserial" to the address of "serialnumber" element */

strcpy(serialnumber, myserial);

/* assign data at serial block address to next indeas of serials array */
serials=serialnumber;

} /* end of array loop */


/*loop through array, print out values of serial numbers */
for (i = 1; i < count; ++i) {
printf("%s\n", serials);
}

/*displose of memory block address for "serialnumber" */

free(serialnumber);


return 0;

}

 

[Jens Thoms Toerring]

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

/* regproc.c--generate serial number list*/

int main (void) {

/* new app */
char appname[] = "MYAPP";

/* pointer variable for serial numbers */
char myserial[100];

/* number of serial numbers */
int count = 20000;

/* index number */
int i;

/* full array of serial numbers */
char *serials[count];

/* length of individual serial number */
int seriallength;

/* placeholder for individual serial number in serials array */
char *serialnumber;

/* character buffer for abbreviated app name: first two letters plus \0
character */
char shortname[3];

/* now trim the application name to the first two letters, copy to
"shortname" array, append to the serial number later */
strncpy(shortname, appname, 2);

That still doesn't add a '\0' to the end of 'shortname' even though
there's now enough space for it. You probably were lucky that
'shortname' was initialized to all zero but you shouldn't bet
on that always to happen. So you have to add

sfortname[ 2 ] = '\0';

/* now build the serial number array */
for (i = 1; i < count; ++i) {
/* second item of the array; first is the app name */
int five = i*5;
/* third item of the array */
int eleven = i/11;
/* fourth item of the array */
int one = (i-1);
/* dump all serial number components to "myserial" character
array--appname, five, eleven, one, shortname */
sprintf(myserial, "%s-%i-%i-%i-%s", appname, five, eleven, one,
shortname);

While all this is fine I probably would shorten that to (but
that's just a matter of taste!):

seriallength = sprintf( myserial, "%s-%i-%i-%i-%.2s", appname,
5 * i, i / 11, i - 1, appname );

sprintf() returns the number of chars it wrote (excluding the
trailing '\0') if it succeeded, so you also don't need the call
of strlen() later.

/* get length of "myserial" string */
seriallength=strlen(myserial);

/* allocate memory for individual serial numbers in array, check for null
*/
serialnumber = (char *) malloc(seriallength);

Here's still a problem. You need one more byte of memory then
what strlen() did return since the trailing (and required)
'\0' at the end of the string isn't included in the count.

Another thing: casting the return value of malloc() isn't
needed in C. It can even be counterproductive since it will
keep the compiler from warning you when you forgot to include

if (serialnumber == NULL) {
return 0; //check for null memory
}

/* copy value of "myserial" to the address of "serialnumber" element */

strcpy(serialnumber, myserial);

/* assign data at serial block address to next indeas of serials array */
serials=serialnumber;

While this is fine in principle it's a bit more work that necessary.
You can also do

if ( ( strings[ i ] = malloc( seriallength + 1 ) ) == NULL )
return 0;
strcpy( serials[ i ], myserial );

So the extra 'serialnumber' variable isn't needed and you can
avoid an extra assignment.

} /* end of array loop */


/*loop through array, print out values of serial numbers */
for (i = 1; i < count; ++i) {
printf("%s\n", serials);
}

/*displose of memory block address for "serialnumber" */

free(serialnumber);

This is also not correct. It only would deallocate the memory
you allocated last, but all the other strings would still be
allocated. Why not simply do

for ( i = 1; i < count; ++i ) {
printf( "%s\n", serials[ i ] );
free( serials[ i ] );
}

That way you get rid of the memory for each string the moment
you don't need it anymore.

Putting everything together I probably would shorten your program
to (the less lines of code you have the less errors you can make;-):

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

#define COUNT 20000

int main ( void ) {
char appname[ ] = "MYAPP";
char myserial[ 100 ];
int i;
char *serials[ COUNT ];
int len;

for ( i = 1; i <= COUNT; i++ ) {
len = sprintf( myserial, "%s-%d-%d-%d-%.2s", appname,
5 * i, i / 11, i - 1, appname );
if ( ( strings[ i - 1 ] = malloc( len + 1 ) ) == NULL )
return 0;
strcpy( serials[ i - 1 ], myserial );
}

for ( i = 0; i < COUNT; ++i ) {
printf( "%s\n", serials[ i ] );
free( serials[ i ] );
}

return 0;
}

I also changed it so that it creates COUNT serial numbers instead
of COUNT - 1 since that looks more logical to me.

Regards, Jens

 

[Ben Bacarisse]

(e-mail address removed) (Jens Thoms Toerring) writes:

(This is reallt to the OP, but it builds on all the correct things
you've said so...)

Kevin Walzer <[email protected]> wrote:

Putting everything together I probably would shorten your program
to (the less lines of code you have the less errors you can make;-):

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

#define COUNT 20000

int main ( void ) {
char appname[ ] = "MYAPP";
char myserial[ 100 ];
int i;
char *serials[ COUNT ];
int len;

for ( i = 1; i <= COUNT; i++ ) {
len = sprintf( myserial, "%s-%d-%d-%d-%.2s", appname,
5 * i, i / 11, i - 1, appname );
if ( ( strings[ i - 1 ] = malloc( len + 1 ) ) == NULL )
return 0;
strcpy( serials[ i - 1 ], myserial );
}

for ( i = 0; i < COUNT; ++i ) {
printf( "%s\n", serials[ i ] );
free( serials[ i ] );
}

return 0;
}

I can't see why the OP needs all the effort of storing these things
just to print them. All that is needed is a loop with your sprintf as
printf. No big array, no mallocs or frees.

Of course, if this is part of some bigger program that needs them all,
then there may be some point, but even then they are not hard to make
on demand.

 

[Kevin Walzer]

Thank you for tweaking the code yet again. I will review it carefully.

 

[Kevin Walzer]

I can't see why the OP needs all the effort of storing these things
just to print them. All that is needed is a loop with your sprintf as
printf. No big array, no mallocs or frees.

Of course, if this is part of some bigger program that needs them all,
then there may be some point, but even then they are not hard to make
on demand.

The obvious reason for my effort is that I am still feeling my way
through C, and have not grasped the best way to do certain things. If I
were an expert, I would not be posting here with questions.

 

[Ben Bacarisse]

The obvious reason for my effort is that I am still feeling my way
through C, and have not grasped the best way to do certain things. If
I were an expert, I would not be posting here with questions.

I hope you did not take offence. When I said I could not see why you
were doing it I was not being snide or sarcastic, just reporting that
I though I was missing some point. Maybe I should have said: "Unless
this is part of larger program, it would be simpler just to print
these rather than store them".

 

[Kevin Walzer]

I hope you did not take offence. When I said I could not see why you
were doing it I was not being snide or sarcastic, just reporting that
I though I was missing some point. Maybe I should have said: "Unless
this is part of larger program, it would be simpler just to print
these rather than store them".

No, no offense taken. Thanks.

 

[Barry Schwarz]

The following code compiles with gcc but returns a bus error when I run
the executable. Any ideas?

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


/* generate serial number list*/
int main (void) {

char appname[] = "MYAPP"; //new app
char myserial[100]; //pointer variable for serial numbers
int count = 20000; //number of serial numbers
int i; //index number;
char *serials[count+1]; //full list of serial numbers
int seriallength; //length of serial number
char *serialblock; //block of memory for serial number

//trim application name
char *shortname = (char *) malloc(2);

Don't cast the return from malloc. It doesn't help anything and can
suppress a diagnostic message you would really want to see.

strncpy(shortname, appname, 2);

This invokes undefined behavior by writing beyond you allocated
memory. appname contains 6 char. You allocated space for 2. The
adage about 10 pounds in a 5 pound bag applies.

... but putting not more than two char of a six-char
string is fine (unless malloc() returned NULL). Note the
use of strncpy() instead of strcpy() -- of course, the two

Yes, even the presence of the third parameter was insufficient for me
to notice the n.

characters { 'M', 'Y' } do not constitute a '\0'-terminated
string, but that scarcely matters here because the next
thing this pointless piece of code does is

free(shortname);

[remainder of helpful analysis snipped]

Remove del for email