Home > database >  Possible double realloc of dynamic struct array elements triggered by fgets, potential struct initia
Possible double realloc of dynamic struct array elements triggered by fgets, potential struct initia

Time：12-24

I've created an array of structs (calloc for initial, realloc for subsequent "elements"). Realloc/initialisation is triggered by each line read from a text file using fgets.

My problem is that I expect a certain amount of struct array elements to be created, but in reality I end up with double the amount. Every second element's data is correctly stored, with empty and/or erroneous data stored in every other element.

Note: This question follows on from a related, but different, problem for the same program: Error: free(): double free detected in tcache 2. Calloc/Realloc Array of Structs in C when triggered by fgets.

The format of the text file per line is:

Single letter   space   single letter   (almost certainly) newline e.g.

A X
C Z
B Y
etc. etc.

Here is the code:

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

#define GROWBY      100
#define BUFFER      100

// The struct definition is:
typedef struct a_struct {
    // Used to store a single char, not sure if there's a better way?
    char firstChar[2];
    char secondChar[2];
    int value;
} TheStruct;

// The initial struct is created thus:
TheStruct *
new_struct(void)
{
    TheStruct *aStruct = NULL;

    aStruct = calloc(1, sizeof(TheStruct)   1);

    return aStruct;
}

// New struct elements are realloc'd dependent on a buffer size, then
// initialised with empty/zeroed data:
TheStruct *
add_struct_to_array(TheStruct * aStruct, int *numElements, int *bufferLen)
{
    char *init = NULL;

    if (*numElements >= *bufferLen) {
        *bufferLen  = GROWBY;
        TheStruct *newStruct = realloc(aStruct, *bufferLen * (sizeof(TheStruct)   1));

        if (newStruct == NULL) {
            free(aStruct);
            printf("Error: Memory could not be allocated.");
            exit(EXIT_FAILURE);
        }
        else if (aStruct != newStruct) {
            aStruct = newStruct;
        }
        newStruct = NULL;
        init = NULL;
    }
    *numElements  = 1;

    char first[2] = { "0" };
    char second[2] = { "0" };
    strcpy(aStruct[*numElements].firstChar, first);
    strcpy(aStruct[*numElements].secondChar, second);
    aStruct[*numElements].value = 0;

    return aStruct;
}

// I then parse a line of string data from the text file to then store in a
// struct element. The two chars per line are used to determine the value of
// the struct:
void
parse_buffer(TheStruct * aStruct, char *buffer, int *numElements)
{
    char charOne[2] = "",
        charTwo[2] = "";

    sscanf(buffer, "%s %s", charOne, charTwo);

    strcpy(aStruct[*numElements].firstChar, charOne);
    strcpy(aStruct[*numElements].secondChar, charTwo);

    if (*charOne == 'A') {
        if (*charTwo == 'X') {
            aStruct[*numElements].value = 6;
        }
        else if (*charTwo == 'Y') {
            aStruct[*numElements].value = 8;
        }
        else if (*charTwo == 'Z') {
            aStruct[*numElements].value = 1;
        }
    }
    else if (*charOne == 'B') {
        if (*charTwo == 'X') {
            aStruct[*numElements].value = 1;
        }
        else if (*charTwo == 'Y') {
            aStruct[*numElements].value = 6;
        }
        else if (*charTwo == 'Z') {
            aStruct[*numElements].value = 8;
        }
    }
    else if (*charOne == 'C') {
        if (*charTwo == 'X') {
            aStruct[*numElements].value = 8;
        }
        else if (*charTwo == 'Y') {
            aStruct[*numElements].value = 1;
        }
        else if (*charTwo == 'Z') {
            aStruct[*numElements].value = 6;
        }
    }
}

// I drive it all through main as so:
void
main()
{
    TheStruct *aStruct = new_struct();
    int structCreated = 0;

    int bufferLen = 0,
        numElements = 0;

    char buffer[BUFFER];

    FILE *file = fopen("input.txt", "r");

    while (fgets(buffer, sizeof(buffer), file) != NULL) {
        if (!structCreated) {
            structCreated = 1;
            numElements  = 1;
        }
        else {
            aStruct = add_struct_to_array(aStruct, &numElements, &bufferLen);
        }
        parse_buffer(aStruct, buffer, &numElements);
    }

    // Free memory
    fclose(file);
    free(aStruct);
}

Valgrind has no problems, but if I print out the data of a struct, I would expect e.g.:

A Y 6

Instead, I get:

  0  
A Y 6

Secondly, if I count the number of times my add_struct_to_array function is called (and thus my expected number of struct array elements), I get double the amount I expect.

I suspect there's a problem in calling add_struct_to_array each time fgets is called - perhaps it gets called twice per line due to the trailing newline as per the following: Removing trailing newline character from fgets() input?

I tried that answer's approach in using strscpn to essentially ignore the newline call, but in doing so while I ended up with the correct number of struct array elements, it'd still be the case that only every second one would be populated with data and every other one would have erroneous data.

CodePudding user response：

A few issues ...

  1. Passing &numElements 1 to add_struct_to_array is UB (undefined behavior). It points to the wrong part of the stack. Just use &numElements
  2. Using structCreated in main just mixes up the indexing. Just initialize aStruct to NULL
  3. Passing sizeof(TheStruct) 1 to malloc/realloc is just trying to mask the indexing issue.
  4. Because add_struct_to_array increments at the end, the returned value is one larger than the index for parse_buffer. We need parse_buffer to use *numElements - 1
  5. In parse_buffer, it's better to get a pointer to the correct element once into a pointer variable rather than doing aStruct[*numElements].whatever everywhere.

Here is the corrected code. It is annotated with bugs and fixes:

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

#ifdef DEBUG
#define dbgprt(_fmt...) \
    printf(_fmt)
#else
#define dbgprt(_fmt...) \
    do { } while (0)
#endif

#define GROWBY      100
#define BUFFER      100

// The struct definition is:
typedef struct a_struct {
    // Used to store a single char, not sure if there's a better way?
    char firstChar[2];
    char secondChar[2];
    int value;
} TheStruct;

// The initial struct is created thus:
TheStruct *
new_struct(void)
{
    TheStruct *aStruct = NULL;

// NOTE/BUG: the "  1" is harmless but incorrect -- same in the realloc below
// if we fix the real bug, this isn't necessary
#if 0
    aStruct = calloc(1, sizeof(TheStruct)   1);
#else
    aStruct = calloc(1, sizeof(TheStruct));
#endif

    return aStruct;
}

// New struct elements are realloc'd dependent on a buffer size, then
// initialised with empty/zeroed data:
TheStruct *
add_struct_to_array(TheStruct *aStruct, int *numElements, int *bufferLen)
{
#if 0
    char *init = NULL;
#endif

    dbgprt("add_struct_to_array: ENTER numElements=%d bufferLen=%d\n",
        *numElements,*bufferLen);

    if (*numElements >= *bufferLen) {
        *bufferLen  = GROWBY;
#if 0
        TheStruct *newStruct = realloc(aStruct,
            *bufferLen * (sizeof(TheStruct)   1));
#else
        TheStruct *newStruct = realloc(aStruct,
            *bufferLen * sizeof(TheStruct));
#endif

        if (newStruct == NULL) {
            free(aStruct);
            printf("Error: Memory could not be allocated.");
            exit(EXIT_FAILURE);
        }
        else if (aStruct != newStruct) {
            aStruct = newStruct;
        }
        newStruct = NULL;
#if 0
        init = NULL;
#endif
    }

// NOTE/BUG: this is misplaced -- the strcpy below will leave array element 0
// alone (and start with 1) -- we want to start with 0
#if 0
    *numElements  = 1;
#endif

// NOTE/BUG: not needed
#if 0
    char first[2] = { "0" };
    char second[2] = { "0" };
    strcpy(aStruct[*numElements].firstChar, first);
    strcpy(aStruct[*numElements].secondChar, second);
    aStruct[*numElements].value = 0;
#endif

#if 1
    *numElements  = 1;
#endif

    dbgprt("add_struct_to_array: EXIT numElements=%d bufferLen=%d\n",
        *numElements,*bufferLen);

    return aStruct;
}

// I then parse a line of string data from the text file to then store in a
// struct element. The two chars per line are used to determine the value of
// the struct:
void
parse_buffer(TheStruct *aStruct, char *buffer, int *numElements)
{
    char charOne[2] = "",
        charTwo[2] = "";

    sscanf(buffer, "%s %s", charOne, charTwo);
    dbgprt("parse_buffer: SCANF '%s' '%s'\n",charOne,charTwo);

#if 1
    dbgprt("parse_buffer: numElements=%d\n",*numElements);
    TheStruct *cur = &aStruct[*numElements - 1];
#endif

    strcpy(cur->firstChar, charOne);
    strcpy(cur->secondChar, charTwo);

#if 1
    cur->value = 0;
#endif

    if (*charOne == 'A') {
        if (*charTwo == 'X') {
            cur->value = 6;
        }
        else if (*charTwo == 'Y') {
            cur->value = 8;
        }
        else if (*charTwo == 'Z') {
            cur->value = 1;
        }
    }
    else if (*charOne == 'B') {
        if (*charTwo == 'X') {
            cur->value = 1;
        }
        else if (*charTwo == 'Y') {
            cur->value = 6;
        }
        else if (*charTwo == 'Z') {
            cur->value = 8;
        }
    }
    else if (*charOne == 'C') {
        if (*charTwo == 'X') {
            cur->value = 8;
        }
        else if (*charTwo == 'Y') {
            cur->value = 1;
        }
        else if (*charTwo == 'Z') {
            cur->value = 6;
        }
    }

    dbgprt("parse_buffer: FINAL firstChar='%s' secondChar='%s' value=%d\n",
        cur->firstChar,cur->secondChar,cur->value);
}

// I drive it all through main as so:
#if 0
void
main()
#else
int
main(void)
#endif
{
#if 0
    TheStruct *aStruct = new_struct();
    int structCreated = 0;
#else
    TheStruct *aStruct = NULL;
#endif

    int bufferLen = 0,
        numElements = 0;

    char buffer[BUFFER];

    FILE *file = fopen("input.txt", "r");

    while (fgets(buffer, sizeof(buffer), file) != NULL) {
// NOTE/BUG: by incrementing before we allocate, add_struct_to_array has the
// wrong count
#if 0
        if (!structCreated) {
            structCreated = 1;
            numElements  = 1;
        }
        else {
#endif
// NOTE/BUG: &numElements   1 is UB (undefined behavior) -- numElements is a
// scalar and _not_ an array -- it points "into the void"
#if 0
            aStruct = add_struct_to_array(aStruct, &numElements   1, &bufferLen);
#else
            aStruct = add_struct_to_array(aStruct, &numElements, &bufferLen);
#endif

#if 0
        }
#endif

        parse_buffer(aStruct, buffer, &numElements);
    }

    // Free memory
    fclose(file);

#if 1
    for (int idx = 0;  idx < numElements;    idx) {
        TheStruct *cur = &aStruct[idx];
        printf("%d: '%s' '%s' %d\n",
            idx,cur->firstChar,cur->secondChar,cur->value);
    }
#endif

    free(aStruct);

#if 1
    return 0;
#endif
}

In the code above, I've used cpp conditionals to denote old vs. new code:

#if 0
// old code
#else
// new code
#endif

#if 1
// new code
#endif

Note: this can be cleaned up by running the file through unifdef -k

For the given input file, here is the program output (with -DDEBUG):

add_struct_to_array: ENTER numElements=0 bufferLen=0
add_struct_to_array: EXIT numElements=1 bufferLen=100
parse_buffer: SCANF 'A' 'X'
parse_buffer: numElements=1
parse_buffer: FINAL firstChar='A' secondChar='X' value=6
add_struct_to_array: ENTER numElements=1 bufferLen=100
add_struct_to_array: EXIT numElements=2 bufferLen=100
parse_buffer: SCANF 'C' 'Z'
parse_buffer: numElements=2
parse_buffer: FINAL firstChar='C' secondChar='Z' value=6
add_struct_to_array: ENTER numElements=2 bufferLen=100
add_struct_to_array: EXIT numElements=3 bufferLen=100
parse_buffer: SCANF 'B' 'Y'
parse_buffer: numElements=3
parse_buffer: FINAL firstChar='B' secondChar='Y' value=6
0: 'A' 'X' 6
1: 'C' 'Z' 6
2: 'B' 'Y' 6

Here is the normal output:

0: 'A' 'X' 6
1: 'C' 'Z' 6
2: 'B' 'Y' 6

Here is the cleaned up program source:

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

#ifdef DEBUG
#define dbgprt(_fmt...) \
    printf(_fmt)
#else
#define dbgprt(_fmt...) \
    do { } while (0)
#endif

#define GROWBY      100
#define BUFFER      100

// The struct definition is:
typedef struct a_struct {
    // Used to store a single char, not sure if there's a better way?
    char firstChar[2];
    char secondChar[2];
    int value;
} TheStruct;

// The initial struct is created thus:
TheStruct *
new_struct(void)
{
    TheStruct *aStruct = NULL;

// NOTE/BUG: the "  1" is harmless but incorrect -- same in the realloc below
    aStruct = calloc(1, sizeof(TheStruct));

    return aStruct;
}

// New struct elements are realloc'd dependent on a buffer size, then
// initialised with empty/zeroed data:
TheStruct *
add_struct_to_array(TheStruct *aStruct, int *numElements, int *bufferLen)
{

    dbgprt("add_struct_to_array: ENTER numElements=%d bufferLen=%d\n",
        *numElements,*bufferLen);

    if (*numElements >= *bufferLen) {
        *bufferLen  = GROWBY;
        TheStruct *newStruct = realloc(aStruct,
            *bufferLen * sizeof(TheStruct));

        if (newStruct == NULL) {
            free(aStruct);
            printf("Error: Memory could not be allocated.");
            exit(EXIT_FAILURE);
        }
        else if (aStruct != newStruct) {
            aStruct = newStruct;
        }
        newStruct = NULL;
    }

// NOTE/BUG: this is misplaced -- the strcpy below will leave array element 0
// alone (and start with 1) -- we want to start with 0

// NOTE/BUG: not needed

    *numElements  = 1;

    dbgprt("add_struct_to_array: EXIT numElements=%d bufferLen=%d\n",
        *numElements,*bufferLen);

    return aStruct;
}

// I then parse a line of string data from the text file to then store in a
// struct element. The two chars per line are used to determine the value of
// the struct:
void
parse_buffer(TheStruct *aStruct, char *buffer, int *numElements)
{
    char charOne[2] = "",
        charTwo[2] = "";

    sscanf(buffer, "%s %s", charOne, charTwo);
    dbgprt("parse_buffer: SCANF '%s' '%s'\n",charOne,charTwo);

    dbgprt("parse_buffer: numElements=%d\n",*numElements);
    TheStruct *cur = &aStruct[*numElements - 1];

    strcpy(cur->firstChar, charOne);
    strcpy(cur->secondChar, charTwo);

    cur->value = 0;

    if (*charOne == 'A') {
        if (*charTwo == 'X') {
            cur->value = 6;
        }
        else if (*charTwo == 'Y') {
            cur->value = 8;
        }
        else if (*charTwo == 'Z') {
            cur->value = 1;
        }
    }
    else if (*charOne == 'B') {
        if (*charTwo == 'X') {
            cur->value = 1;
        }
        else if (*charTwo == 'Y') {
            cur->value = 6;
        }
        else if (*charTwo == 'Z') {
            cur->value = 8;
        }
    }
    else if (*charOne == 'C') {
        if (*charTwo == 'X') {
            cur->value = 8;
        }
        else if (*charTwo == 'Y') {
            cur->value = 1;
        }
        else if (*charTwo == 'Z') {
            cur->value = 6;
        }
    }

    dbgprt("parse_buffer: FINAL firstChar='%s' secondChar='%s' value=%d\n",
        cur->firstChar,cur->secondChar,cur->value);
}

// I drive it all through main as so:
int
main(void)
{
    TheStruct *aStruct = NULL;

    int bufferLen = 0,
        numElements = 0;

    char buffer[BUFFER];

    FILE *file = fopen("input.txt", "r");

    while (fgets(buffer, sizeof(buffer), file) != NULL) {
// NOTE/BUG: by incrementing before we allocate, add_struct_to_array has the
// wrong count
// NOTE/BUG: &numElements   1 is UB (undefined behavior) -- numElements is a
// scalar and _not_ an array -- it points "into the void"
            aStruct = add_struct_to_array(aStruct, &numElements, &bufferLen);

        parse_buffer(aStruct, buffer, &numElements);
    }

    // Free memory
    fclose(file);

    for (int idx = 0;  idx < numElements;    idx) {
        TheStruct *cur = &aStruct[idx];
        printf("%d: '%s' '%s' %d\n",
            idx,cur->firstChar,cur->secondChar,cur->value);
    }

    free(aStruct);

    return 0;
}

CodePudding user response：

The trouble for me is that this code shows a lot of "copy/paste/adapt" to grow the program without first having attended to good programming practices.

Below is an untested re-write of most of the code. Notice the use of short but meaningful variable names. Don't fog up the code with verbiage. Programmers are not paid by the kilogram of code.

realloc() acts like malloc() when passed a null pointer. There's no need to "grab the first one" as if it were special. Start from nothing and build from there.

That if/else ladder is an invitation for bugs to surface. The ellipses used in your question suggest you want to extend this to (perhaps many) more letter pairs. You'll go blind trying to copy/paste/adapt that if/else and no-one will want to read it.

The following shows how you might achieve your objective with far less code.

void fill( TheStruct *p, char *str ) {
    char t0 = p->firstChar = str[0];
    char t1 = p->secondChar = str[0   2]; // NB!!

    struct {
        char t0, t1;
        int val;
    } LUT[] = {
        { 'A', 'X', 6 }, { 'A', 'Y', 8 }, { 'A', 'Z', 1 },
        { 'B', 'X', 1 }, { 'B', 'Y', 6 }, { 'B', 'Z', 8 },
        { 'C', 'X', 8 }, { 'C', 'Y', 1 }, { 'C', 'Z', 6 },
    };
    const int nLUT = sizeof LUT/sizeof LUT[0];

    for( size_t i = 0; i < nLUT; i   )
        if( t0 == LUT[i].t0 && t1 == LUT[i].t1 ) {
            p->value = LUT[i].val;
            break;
        }
    // Deal with NOT finding the user input to be as expected.
}

TheStruct *add( TheStruct *p, int *pUsed, int *pSz ) {
    if( *pUsed >= *pSz ) {
        *pSz  = GROWBY; // Going to exit if fail so 
        if( (p = realloc( p, *pSz * sizeof *p ) ) == NULL) {
            // free(aStruct); // no need if terminating. OS cleans up.
            puts( "Error: Memory could not be allocated." );
            exit( EXIT_FAILURE );
        }
    }
    memset( p[ *pUsed   ], '\0', sizeof *p ); // new struct is zero'd out
    return p;
}

void main() {
    TheStruct *pSt = NULL;
    int used = 0, size = 0;
    char buffer[BUFFER];

    FILE *fp = fopen( "input.txt", "r" );
    /* omitting test for success */

    while( fgets( buffer, sizeof buffer , fp ) ) {
        pSt = add( pSt, &used, &size );

        fill( pSt   used - 1, buffer );
    }
    fclose( fp );

    free( pSt );
}

There are alternatives in the way this LUT (look up table) has been implemented. Maybe you will want to explore some of those...

Page link：https//www.codepudding.com/database/652563.html

Prev:Why is my %d specifier putting the "-" after the number?
Next:Parsing The Data from Rasberry Pi to Arduino for controlling 8 Relay
  • Related

About Us:  Contact Us      Terms of Service       Privacy Policy

Copyright © 2010-2023，Powered By CodePudding