Bus error--not sure why

Discussion in 'C Programming' started by Kevin Walzer, Jul 25, 2008.

  1. Kevin Walzer

    Kevin Walzer Guest

    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;



    }
     
    Kevin Walzer, Jul 25, 2008
    #1
    1. Advertisements

  2. Why 'count+1' when your lop only runs from 1 to 'count-1'?
    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.
    Are you sure you're getting a bus error (I guess that means a SIGBUS)
    instead of a segmentation fault (SIGSEGV)?

    Regards, Jens
     
    Jens Thoms Toerring, Jul 25, 2008
    #2
    1. Advertisements

  3. Don't cast the return from malloc. It doesn't help anything and can
    suppress a diagnostic message you would really want to see.
    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.
    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.
    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.
    It is a little strange to return success when your program failed.
    This is why you should not use // comments in a usenet post.


    This releases the memory that serial points to.


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

    Remove del for email
     
    Barry Schwarz, Jul 25, 2008
    #3
  4. Kevin Walzer

    Kevin Walzer Guest



    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;

    }
     
    Kevin Walzer, Jul 25, 2008
    #4
  5. 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';
    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.
    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


    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.


    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
     
    Jens Thoms Toerring, Jul 25, 2008
    #5
  6. (Jens Thoms Toerring) writes:

    (This is reallt to the OP, but it builds on all the correct things
    you've said so...)
    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.
     
    Ben Bacarisse, Jul 25, 2008
    #6
  7. Kevin Walzer

    Kevin Walzer Guest

    Thank you for tweaking the code yet again. I will review it carefully.
     
    Kevin Walzer, Jul 25, 2008
    #7
  8. Kevin Walzer

    Kevin Walzer Guest

    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.
     
    Kevin Walzer, Jul 25, 2008
    #8
  9. 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".
     
    Ben Bacarisse, Jul 25, 2008
    #9
  10. Kevin Walzer

    Kevin Walzer Guest

    No, no offense taken. Thanks.
     
    Kevin Walzer, Jul 26, 2008
    #10
  11. Yes, even the presence of the third parameter was insufficient for me
    to notice the n.

    Remove del for email
     
    Barry Schwarz, Jul 26, 2008
    #11
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.