1

I'm working on a school project where I have to store PPM data into structs. I have an issue with an array of strings inside the struct.

typedef struct {
     char **comments;
} PPM;

I have 3 functions that uses this struct.

  1. PPM *getPPM() is used to get all the PPM data from file and store it into the struct

  2. void showPPM(PPM * img) to show the image data in terminal

  3. PPM *encode(PPM *img) which is used to change the LSB of the RGB values of the image

The problem is that getPPM works as intended and gets all the comments into the comments array in getPPM. It displays them fine if I do it like this:

PPM *p = getPPM(fin);
showPPM(p);

But if I try to call it with the encode function like this:

PPM *p = getPPM(fin);
PPM *g = encode(p);
showPPM(g);

the debugger shows that as soon as the program enters the encode function, the comments value resets to NULL even though this function doesn't even touch comments. Is the way I am calling these functions wrong or is there something wrong with my code? I will try to provide the minimal code if the problem is not the way the functions are being called, as the code is big and everything is dependent on one another.

I'm very new to C language. I tried understanding the problem for hours but can't find a solution anywhere. Any help would be greatly appreciated.

EDIT: This is as small as I could make it.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
//Structures
typedef struct {
    int r, g, b;
} pixels;

typedef struct {
    char format[3];
    char **comments;
    int width, height, maxColors, commentCounter;
    pixels **pixelValues;
} PPM;
// Functions declarations
PPM *getPPM(FILE * f);
PPM *encode(PPM *im, char *message, unsigned int mSize, unsigned int secret);
void showPPM(PPM * im);
static int *decimalToBinary(const char *message, unsigned int length);
// Main method
int main(int argc, char **argv) {

    FILE * fin = fopen(argv[1], "r");

    if(fin == NULL) {
        perror("Cannot open file");
        exit(1);
    }

    PPM *p = getPPM(fin);
    PPM *g = encode(p, "test", 5, 1);
    showPPM(g);

    return 0;
}
/*
 * This function is used to get the image data from a file and populate
 * our strutures with it.
 */
PPM *getPPM(FILE * f) {

    // Allocate the memory for structure and check if it was successful
    PPM *pic = (PPM *) malloc(sizeof(PPM));

    if(!pic) {
        perror("Unable to allocate memory for structure");
        exit(1);
    }

    char line[100]; // Expecting no more than 100 characters per line.
    pic->commentCounter = 0; // This is the counter to keep size if there are more than one comments
    int pixelsCounter = 0; // Counter for pixels' array
    pic->comments = malloc(sizeof(char *));
    pic->pixelValues = malloc(sizeof(PPM));
    int lineCounter = 0;

    if((pic->comments) == NULL) {
        perror("Unable to allocate memory for pixels");
        exit(1);
    }

    while(fgets(line, sizeof(line), f)) {
        // Reference: https://stackoverflow.com/questions/2693776/removing-trailing-newline-character-from-fgets-input
        size_t length = strlen(line);

        if(length > 0 && line[length-1] == '\n') {
            line[--length] = '\0';
        }
        // Assigning the file format
        if(line[0] == 'P') {
            pic->format[0] = line[0];
            pic->format[1] = line[1];
            pic->format[2] = '\0';
        }
        //Populate comments into struct PPM
        if(line[0] == '#') {
            // Reallocate/allocate the array size each time a new line of comment is found
            if(pic->commentCounter != 0) {
                pic->comments = realloc(pic->comments, (pic->commentCounter+1) * sizeof(char *));
            }
            // Allocate the memory for the string
            pic->comments[pic->commentCounter] = malloc(100 * sizeof(char));
            // Write the at commentCounter position of the array; character by character
            int i = 0;
            while(line[i] != '\0') {
                pic->comments[pic->commentCounter][i] = line[i];
                i++;
            }
            pic->comments[pic->commentCounter][i] = '\0';
            pic->commentCounter++;
        }
        /*
         * Loading the max color property of the file which is gonna be 3 letters (Usually 255)
         * and checking if we previously got maxColors in our construct or not. If we didn't
         * then we load this value into the consturct and the condition will never validate
         * throughout the iterations
         */
        if(strlen(line) == 3 && pic->maxColors == 0 && line[0] != '#') {
            pic->maxColors = atoi(line);
            continue;

        }
        /*
         * Check if the length of string > 3, which means it is going to be a
         * number, potentially RGB value or a comment. But since width & height
         * comes before RGB values, our condition will fail once we have found
         * the width/height for the next iteration. That's why this condition
         * only checks if it is a comment or a numbered value of length > 3
         */
        if((strlen(line) > 3) && (pic->width == 0) && (line[0] != '#')) {
            char *width = strtok(line, " ");
            char *height = strtok(NULL, " ");

            pic->width = atoi(width);
            pic->height = atoi(height);
            continue;
        }
        /*
         * If the width/height and maxColors have been found, that means every
         * other line is either going to be the RGB values or a comment.
         */
        if((pic->width != 0) && (pic->maxColors != 0) && (line[0] != '#')) {
            // length(line) > 3 means all the RGB values are in same line
            if(strlen(line) > 3) {
                char *val1 = strtok(line, " ");
                char *val2 = strtok(NULL, " ");
                char *val3 = strtok(NULL, " ");
                // pixelsCounter = 0 means it's the first element.
                if(pixelsCounter != 0) {
                    // Reallocate memory each time a new R G B value line is found
                    pic->pixelValues = realloc(pic->pixelValues, (pixelsCounter + 1) * sizeof(PPM));
                }

                pic->pixelValues[pixelsCounter] = malloc(12 * sizeof(pixels));
                pic->pixelValues[pixelsCounter]->r = atoi(val1);
                pic->pixelValues[pixelsCounter]->g = atoi(val2);
                pic->pixelValues[pixelsCounter]->b = atoi(val3);
                pixelsCounter++;
            } else if(strlen(line) <= 3) {
                /*
                 * If each individual RGB values are in a separete lines, we will
                 * use a switch case and a line counter to keep track of where the
                 * values were inserted and when to know when we got RGB values for
                 * one pixel
                 */
                if(pixelsCounter != 0 && lineCounter == 0) {
                    // Reallocate memory each time a new R G B value line is found
                    pic->pixelValues = realloc(pic->pixelValues, (pixelsCounter + 1) * sizeof(PPM));
                }

                switch(lineCounter) {
                    case 0 :
                        pic->pixelValues[pixelsCounter] = malloc(12 * sizeof(pixels));
                        pic->pixelValues[pixelsCounter]->r = atoi(line);
                        lineCounter++;
                        continue;
                    case 1 :
                        pic->pixelValues[pixelsCounter]->g = atoi(line);
                        lineCounter++;
                        continue;
                    case 2 :
                        pic->pixelValues[pixelsCounter]->b = atoi(line);
                        lineCounter=0;
                        pixelsCounter++;
                        continue;
                    default:
                        continue;
                }
            }
        }
    }
    pic->pixelValues[pixelsCounter] = NULL;
    fclose(f);
    return pic;
}

void showPPM(PPM * im) {

    printf("%s\n",im->format);
    int k = 0;
    while(k < im->commentCounter) {
        printf("%s\n", im->comments[k]);
        k++;
    }

    printf("%d %d\n", im->width, im->height);
    printf("%d\n",im->maxColors);

    int j = 0;
    while(im->pixelValues[j] != NULL) {
        printf("%d %d %d\n", im->pixelValues[j]->r, im->pixelValues[j]->g, im->pixelValues[j]->b);
        j++;
    }
}


PPM *encode(PPM *im, char *message, unsigned int mSize, unsigned int secret) {

    int *binaryMessage = decimalToBinary(message, mSize);
    int i, j = 0, lineCounter = 0;

    for(i = 0; i < 40; i++) {
        switch(lineCounter) {
            case 0 :
                im->pixelValues[j]->r |= binaryMessage[i] << 0;
                lineCounter++;
                continue;
            case 1 :
                im->pixelValues[j]->g |= binaryMessage[i] << 0;
                lineCounter++;
                continue;
            case 2 :
                im->pixelValues[j]->b |= binaryMessage[i] << 0;
                lineCounter=0;
                j++;
                continue;
            default:
                continue;
        }

    }
    return im;
}
/*
 * Converts a string into binary to be used in encode function. It
 * first converts each letter of the string into ascii code. Then
 * finds and stores each of the 8 bits of that int (ascii code of
 * the letter) sequentially in an array.
 */
static int *decimalToBinary(const char *message, unsigned int length) {
    /*
     * malloc is used here instead of [] notation to allocate memory,
     * because defining the variable with [] will make its scope
     * limited to this function only. Since we want to access this
     * array later on, we use malloc to assign space in the memory
     * for it so we can access it using a pointer later on.
     */
    int k=0, i, j;
    unsigned int c;
    unsigned int *binary = malloc(8 * length);

    for(i = 0; i < length; i++) {
        c = message[i];
        for(j = 7; j >= 0; j--,k++) {
            /*
             * We check here if the jth bit of the number is 1 or 0
             * using the bit operator &. If it is 1, it will return
             * 1 because 1 & 1 will be true. Otherwise 0.
             */
            if((c >> j) & 1)
                binary[k] = 1;
            else
                binary[k] = 0;
        }
    }
    return binary;
}

PPM file:

P3
# CREATOR: GIMP PNM Filter Version 1.1
# Amazing comment 2
# Yet another amazing comment
400 530
255
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
0 0 0
Naeem Khan
  • 950
  • 4
  • 13
  • 34
  • This problem is often caused due to the fact that C parameters are always passed by value, and if your pointer is NULL when you pass it into your function, even if you change it inside the function, it will continue to be NULL after leaving the function. – bruceg Feb 24 '19 at 17:21
  • You mention that the code is long and intertwined. When coding always break the code into simple, straight forward, short functions. A major concept in coding is that individual functions do not depend on data that is not passed to the function. Your description reads like spaghetti code. – user3629249 Feb 24 '19 at 17:26
  • Hi. I have updated the post with as minimal working code as I could. I did break each task into smaller parts, I moved to the encoding part once showPPM was working. But as soon as I ran the encode with it, it stopped working and I'm completely clueless as to where the problem is. – Naeem Khan Feb 24 '19 at 17:29
  • 1
    Your weird reimplementation of `strcpy` does not terminate the target string. – melpomene Feb 24 '19 at 17:29
  • @melpomene shit I just noticed that. Thank you for clarifying. I have fixed that but still I have the same problem. – Naeem Khan Feb 24 '19 at 17:32
  • I can't reproduce the issue. Unfortunately I can't post the link to the live demo because StackOverflow is being dumb (disallows long URLs, disallows use of URL shorteners). – melpomene Feb 24 '19 at 17:34
  • may be give the ppm file you use ? – bruno Feb 24 '19 at 17:37
  • @bruno I have updated the post with PPM file I'm using. – Naeem Khan Feb 24 '19 at 17:41
  • I must clarify I'm using CentOS 7 and working on Clion IDE. – Naeem Khan Feb 24 '19 at 17:41
  • using your file I see nothing wrong after adding a final null char in `pic->comments[pic->commentCounter]` in _getPPM_, nothing from _valgrind_ – bruno Feb 24 '19 at 17:46
  • @bruno pic->comments[pic->commentCounter] part itself adds the comments fine. But when I call the functions in main method like this PPM *p = getPPM(fin); PPM *g = encode(p); showPPM(g); is when the problem occurs. As soon as it reaches the encode function the value resets to NULL. Should I post the entire code? – Naeem Khan Feb 24 '19 at 17:50
  • 1
    @NaeemKhan do you have the problem with the reduced code you currently give here ? – bruno Feb 24 '19 at 17:51
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/188957/discussion-between-naeem-khan-and-bruno). – Naeem Khan Feb 24 '19 at 17:53
  • Unfortunately yes. – Naeem Khan Feb 24 '19 at 17:59
  • with the new version valgrind signals a lot of errors, including invalid writes – bruno Feb 24 '19 at 18:42
  • Sorry. Please get the new PPM file. It was trying to write 40 pixels but I had provided only 10. – Naeem Khan Feb 24 '19 at 18:44
  • same problem with the new ppm file – bruno Feb 24 '19 at 18:44
  • did you run with valgrind ? – bruno Feb 24 '19 at 18:45
  • @bruno Yeah. It doesn't give me those errors that you have shown in discussion chat. – Naeem Khan Feb 24 '19 at 18:51
  • 1
    OK, problems solved after a loooooooooooooooooong chat – bruno Feb 24 '19 at 19:29

1 Answers1

1

in decimalToBinar

unsigned int *binary = malloc(8 * length);

must be

unsigned int *binary = malloc(8 * length * sizeof(int));

new code is :

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
//Structures
typedef struct {
    int r, g, b;
} pixels;

typedef struct {
    char format[3];
    char **comments;
    int width, height, maxColors, commentCounter;
    pixels **pixelValues;
} PPM;
// Functions declarations
PPM *getPPM(FILE * f);
PPM *encode(PPM *im, char *message, unsigned int mSize, unsigned int secret);
void showPPM(PPM * im);
static int *decimalToBinary(const char *message, unsigned int length);
// Main method
int main(int argc, char **argv) {

    FILE * fin = fopen(argv[1], "r");

    if(fin == NULL) {
        perror("Cannot open file");
        exit(1);
    }

    PPM *p = getPPM(fin);
    PPM *g = encode(p, "test", 5, 1);
    showPPM(g);

    free(p->comments);
    free(p);

    return 0;
}
/*
 * This function is used to get the image data from a file and populate
 * our strutures with it.
 */
PPM *getPPM(FILE * f) {

    // Allocate the memory for structure and check if it was successful
    PPM *pic = (PPM *) malloc(sizeof(PPM));

    if(!pic) {
        perror("Unable to allocate memory for structure");
        exit(1);
    }

    char line[100]; // Expecting no more than 100 characters per line.
    pic->commentCounter = 0; // This is the counter to keep size if there are more than one comments
    int pixelsCounter = 0; // Counter for pixels' array
    pic->comments = malloc(sizeof(char *));
    pic->pixelValues = malloc(sizeof(PPM));
    int lineCounter = 0;

    if((pic->comments) == NULL) {
        perror("Unable to allocate memory for pixels");
        exit(1);
    }
    pic->width = 0; 
    pic->height = 0; 
    pic->maxColors = 0;
    while(fgets(line, sizeof(line), f)) {
        // Reference: https://stackoverflow.com/questions/2693776/removing-trailing-newline-character-from-fgets-input
        size_t length = strlen(line);

        if(length > 0 && line[length-1] == '\n') {
            line[--length] = '\0';
        }
        // Assigning the file format
        if(line[0] == 'P') {
            pic->format[0] = line[0];
            pic->format[1] = line[1];
            pic->format[2] = '\0';
        }
        //Populate comments into struct PPM
        if(line[0] == '#') {
            // Reallocate/allocate the array size each time a new line of comment is found
            if(pic->commentCounter != 0) {
                pic->comments = realloc(pic->comments, (pic->commentCounter+1) * sizeof(char *));
            }
            // Allocate the memory for the string
            pic->comments[pic->commentCounter] = malloc(100 * sizeof(char));
            // Write the at commentCounter position of the array; character by character
            int i = 0;
            while(line[i] != '\0') {
                pic->comments[pic->commentCounter][i] = line[i];
                i++;
            }
            pic->comments[pic->commentCounter][i] = '\0';
            pic->commentCounter++;
        }
        /*
         * Loading the max color property of the file which is gonna be 3 letters (Usually 255)
         * and checking if we previously got maxColors in our construct or not. If we didn't
         * then we load this value into the consturct and the condition will never validate
         * throughout the iterations
         */
        if(strlen(line) == 3 && pic->maxColors == 0 && line[0] != '#') {
            pic->maxColors = atoi(line);
            continue;

        }
        /*
         * Check if the length of string > 3, which means it is going to be a
         * number, potentially RGB value or a comment. But since width & height
         * comes before RGB values, our condition will fail once we have found
         * the width/height for the next iteration. That's why this condition
         * only checks if it is a comment or a numbered value of length > 3
         */
        if((strlen(line) > 3) && (pic->width == 0) && (line[0] != '#')) {
            char *width = strtok(line, " ");
            char *height = strtok(NULL, " ");

            pic->width = atoi(width);
            pic->height = atoi(height);
            continue;
        }
        /*
         * If the width/height and maxColors have been found, that means every
         * other line is either going to be the RGB values or a comment.
         */
        if((pic->width != 0) && (pic->maxColors != 0) && (line[0] != '#')) {
            // length(line) > 3 means all the RGB values are in same line
            if(strlen(line) > 3) {
                char *val1 = strtok(line, " ");
                char *val2 = strtok(NULL, " ");
                char *val3 = strtok(NULL, " ");
                // pixelsCounter = 0 means it's the first element.
                if(pixelsCounter != 0) {
                    // Reallocate memory each time a new R G B value line is found
                    pic->pixelValues = realloc(pic->pixelValues, (pixelsCounter + 1) * sizeof(PPM));
                }

                pic->pixelValues[pixelsCounter] = malloc(12 * sizeof(pixels));
                pic->pixelValues[pixelsCounter]->r = atoi(val1);
                pic->pixelValues[pixelsCounter]->g = atoi(val2);
                pic->pixelValues[pixelsCounter]->b = atoi(val3);
                pixelsCounter++;
            } else if(strlen(line) <= 3) {
                /*
                 * If each individual RGB values are in a separete lines, we will
                 * use a switch case and a line counter to keep track of where the
                 * values were inserted and when to know when we got RGB values for
                 * one pixel
                 */
                if(pixelsCounter != 0 && lineCounter == 0) {
                    // Reallocate memory each time a new R G B value line is found
                    pic->pixelValues = realloc(pic->pixelValues, (pixelsCounter + 1) * sizeof(PPM));
                }

                switch(lineCounter) {
                    case 0 :
                        pic->pixelValues[pixelsCounter] = malloc(12 * sizeof(pixels));
                        pic->pixelValues[pixelsCounter]->r = atoi(line);
                        lineCounter++;
                        continue;
                    case 1 :
                        pic->pixelValues[pixelsCounter]->g = atoi(line);
                        lineCounter++;
                        continue;
                    case 2 :
                        pic->pixelValues[pixelsCounter]->b = atoi(line);
                        lineCounter=0;
                        pixelsCounter++;
                        continue;
                    default:
                        continue;
                }
            }
        }
    }
    pic->pixelValues[pixelsCounter] = NULL;
    fclose(f);
    return pic;
}

void showPPM(PPM * im) {

    printf("%s\n",im->format);
    int k = 0;
    while(k < im->commentCounter) {
        printf("%s\n", im->comments[k]);
        k++;
    }

    printf("%d %d\n", im->width, im->height);
    printf("%d\n",im->maxColors);

    int j = 0;
    while(im->pixelValues[j] != NULL) {
        printf("%d %d %d\n", im->pixelValues[j]->r, im->pixelValues[j]->g, im->pixelValues[j]->b);
        j++;
    }
}


PPM *encode(PPM *im, char *message, unsigned int mSize, unsigned int secret) {

    int *binaryMessage = decimalToBinary(message, mSize);
    int i, j = 0, lineCounter = 0;

    for(i = 0; i < 40; i++) {
        switch(lineCounter) {
            case 0 :
                im->pixelValues[j]->r |= binaryMessage[i] << 0;
                lineCounter++;
                continue;
            case 1 :
                im->pixelValues[j]->g |= binaryMessage[i] << 0;
                lineCounter++;
                continue;
            case 2 :
                im->pixelValues[j]->b |= binaryMessage[i] << 0;
                lineCounter=0;
                j++;
                continue;
            default:
                continue;
        }

    }
    free(binaryMessage);
    return im;
}
/*
 * Converts a string into binary to be used in encode function. It
 * first converts each letter of the string into ascii code. Then
 * finds and stores each of the 8 bits of that int (ascii code of
 * the letter) sequentially in an array.
 */
static int *decimalToBinary(const char *message, unsigned int length) {
    /*
     * malloc is used here instead of [] notation to allocate memory,
     * because defining the variable with [] will make its scope
     * limited to this function only. Since we want to access this
     * array later on, we use malloc to assign space in the memory
     * for it so we can access it using a pointer later on.
     */
    int k = 0, i, j;
    unsigned int c;
    unsigned int *binary = malloc(8 * length * sizeof(int));

    for(i = 0; i < length; i++) {
        c = message[i];
        for(j = 7; j >= 0; j--,k++) {
            /*
             * We check here if the jth bit of the number is 1 or 0
             * using the bit operator &. If it is 1, it will return
             * 1 because 1 & 1 will be true. Otherwise 0.
             */
            if((c >> j) & 1)
                binary[k] = 1;
            else
                binary[k] = 0;
        }
    }
    return binary;
}
bruno
  • 32,421
  • 7
  • 25
  • 37