Steganography on PPM images

Question

I have recently finished a project on stenography on P3 images and just wanted some advice if there was any improvements I could make on my code. I am new to C so not sure if what I am doing is the best practice so just wanting to learn!

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

/*Using a Link list as the primary Data Structure */

/*Part A Structures*/

//Create the linked list
typedef struct NODE 
{
char *val;
struct NODE *next;
}NODE;

//Hold the Pixel of the image
typedef struct PIXEL 
{
 int R, G, B;
}PIXEL;

//Hold the PPM Image
typedef struct PPM 
{
char format[2];;
NODE *comments;
int w, h;
int max;
PIXEL * arr;
}PPM;

/**
* function used to deepcopy the linkedlist holding the comments.
* it returns a pointer to a deepcopy of the list passed as a parameter;
*/
//Copy the linked list
//Points to the copy
NODE *copy(NODE *first) 
{
NODE *second = NULL, *previous = NULL;

while (first != NULL) 
{
NODE * temp = (NODE *) malloc(sizeof(NODE));
temp->val = first->val;
temp->next = NULL;

if (second == NULL) 
{
    second = temp;
    previous = temp;
} 
else 
{
    previous->next = temp;
    previous = temp;
}
first = first->next;
}
return second;
}


//Copy the PPM File Returning Pointer
struct PPM * createPPM(PPM *old) 
 {
PPM *new = (PPM*) malloc(sizeof(PPM));

strcpy(new->format, old->format); 
new->comments = copy(old->comments); 

new->h = old->h;
new->w = old->w;
new->max = old->max;

new->arr = (PIXEL *) malloc(old->h * old->w * sizeof(PIXEL));
memcpy(new->arr, old->arr, old->h * old->w * sizeof(PIXEL));

return new;
}

/*Part B */


//Function to read a PPM file and store the structure
struct PPM * getPPM(FILE * fd) 
{
PPM *image = (PPM *) malloc(sizeof(PPM));

fscanf(fd, "%s", image->format);
if (strcmp(image->format, "P3") != 0) 
{
printf("Invalid Image Type");
exit(0);
}
char c = getc(fd);
image->comments = (NODE *) malloc(sizeof(NODE));
NODE *temp = image->comments;
 while ((c = getc(fd)) == '#') 
{
fseek(fd, -1, SEEK_CUR); 
char str[50];
fgets(str, 50, fd);
temp->val = (char *) malloc(strnlen(str, 50));
strcpy(temp->val, str);
temp->val[strlen(temp->val) - 1] = 0;
temp->next = (NODE *) malloc(sizeof(NODE));
temp = temp->next;
temp->next = NULL;
}

fseek(fd, -1, SEEK_CUR);
fscanf(fd, "%d", &image->w);
fscanf(fd, "%d", &image->h);
fscanf(fd, "%d", &image->max);

image->arr = (PIXEL *) malloc(image->h * image->w * sizeof(PIXEL));

int t = 0;
int j = 0;

while (j < image->h * image->w)
{
t = fscanf(fd, "%d", &image->arr[j].R);
t = fscanf(fd, "%d", &image->arr[j].G);
t = fscanf(fd, "%d", &image->arr[j].B);
j = j + 1;
}

return image;
}

//Encode The message into the PPM file
struct PPM * encode(char * text, struct PPM * i) 
{
PPM * str = createPPM(i); 
int random;
srand((unsigned) time(NULL));
int randMax = (i->h * i->w) / (strlen(text)+1);

random = rand() % randMax; 

if(random<1)
{
random = 1; 
}

int k =0;
int j = random;

//Red fields swapped with ASCII int
while (k < strlen(text)) 
{
if(str->arr[j].R == text[k])
{
    j = j+1; // if the values are the same we encode in the next  pixel.
}
else
{
str->arr[j].R = text[k]; 
k = k+1;
j = j + random;
}
}

return str;
}

//Function Decodes the messages comparing the new copy to the original
char * decode(struct PPM * i1, struct PPM * i2) 
{
int i = 0;
int j = 0;

char *str = (char *) malloc(255);

while (i < i1->h * i1->w)
{
if(i1->arr[i].R != i2->arr[i].R)
{
str[j] = i2->arr[i].R;
j = j+1;
}
i = i + 1;
}

str = realloc(str,i);

return str;
}

/**
* function to print the PPM structure
*/
void showPPM(struct PPM * i) 
{
printf("%s\n", i->format); //print format

//print comments
NODE *n = i->comments;
while (n->next != NULL)
{
printf("%s\n", n->val);
n = n->next;
}

//print width, height and max
printf("%d %d\n%d\n", i->w, i->h, i->max);

//print the array containing the pixels
int j;
for (j = 0; j < i->h * i->w; ++j) 
{
printf("%d %d %d\n", i->arr[j].R, i->arr[j].G, i->arr[j].B);
}

return;
}

int main (int argc, char ** argv)
{

if(argc != 3 && argc != 4)
{ 
printf("Encode 3 arguments, Decode 4\n");
exit(0);
}

else if(argc ==3)
{   
if(strncmp(argv[1], "e",1) != 0)
{ 
    printf("non valid commands %s for %d arguments\n", argv[1], argc);
    exit(0);
}

FILE * f = fopen(argv[2], "r");
PPM * ppm1 = getPPM(f);
fclose(f);
char * toEnc =(char*) malloc(255 * sizeof(char));
fgets (toEnc, 255, stdin);
PPM * encoded = encode(toEnc, ppm1);
showPPM(encoded);
}

else
{
if(strncmp(argv[1], "d",1) != 0)
{
    printf("non valid commands %s for %d arguments\n", argv[1], argc);
    exit(0);    
}
FILE * file1 = fopen(argv[2], "r");
FILE * file2 = fopen(argv[3], "r");
PPM * ppm1 = getPPM(file1);
PPM * ppm2 = getPPM(file2);
char * dec = decode(ppm1, ppm2);
printf("%s", dec); 
}

}

Answer 1

It is best to separate the definition of a struct from the definition of a typedef

So this:

typedef struct NODE
{
    char *val;
    struct NODE *next;
}NODE;

would be better written as:

struct NODE
{
    char *val;
    struct NODE *next;
};

typedef struct NODE NODE;

Similar considerations apply to PIXEL and PPM for readability and ease of understanding:

1. Separate code blocks (for, if, else, while, do...while, switch, case, default) via a blank line.

2. Separate functions by 2 or 3 blank lines (be consistent)

3. Consistently indent the code:

   • Indent after every opening brace '{'
   • Unindent before every closing brace '}'
   • Suggest each indent level be 4 spaces as that is wide enough to be visible even with variable width fonts

For assureance that a call to a system function was successful, check the returned value: when calling: malloc(), fscanf(), realloc()m, fopen(), fseek() to avoid memory leaks, for each call to a heap memory allocation function (slightly more complicated for realloc(), there needs to be a call to free() for that same memory pointer.

The function srand() should only be called ONCE in the whole program. Suggest calling srand() very early in the main() function.

When exiting the program due to an error, do not use a return value of 0 as that is used to indicate success. Suggest using EXIT_FAILURE (defined in the stdlib.h header fileno. It is considered best to (when making a comparison using the == operator) to place the literal on the left, so a keypunch error like typing = rather than == will be be caught by the compiler (else if( 3 == argc )).

When outputting an error message, it should be output to stderr, not stdout, so use either:

fprintf( stderr, "...");

or if due to a system function use:

perror "..." );

perror() will also output the text from the OS as to why the OS thinks the error occurred.

The memory allocation functions (malloc, realloc, calloc) return a pointer with type void*, so can be assigned to any other pointer. Casting the returned value just clutters the code, making it more difficult to understand, debug, maintain.

PPM images have to have a multiple of 4 bytes on each row, so there is often some 'dummy' bytes at the end of each row, The posted code is failing to allow for that condition.

The function getc() returns an int, not a char and the returned value should always be checked for EOF.

When compiling, always enable all the warnings, then fix those warnings. (for gcc, at a minimum use: -Wall -Wextra -pedantic. I also use: -Wconversion -std=gnu11).

Variable and parameter names should indicate content or usage (or better, both). Parameter names like i are meaningless, even in the current context

Regarding this line:

temp->val = (char *) malloc(strnlen(str, 50));

The function strnlen() will not allow enough room for the trailing NUL byte, so the line should be:

temp->val = malloc(strnlen(str, 50)+1);

This line:

temp->val[strlen(temp->val) - 1] = 0;

will chop off the last character of the string, and this line is not needed at all because the prior call to strcpy() will include a trailing NUL byte.

Regarding this field in the struct PPM:

char format[2];;

The method of setting that field will include a trailing NUL byte and perhaps even a newline (which the code should be overlaying the newline with a NUL char) so the line should be: (including removing the extra ;)

char format[3];

There are plenty more problems but the above should get you started.

#endif





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

/*Using a Link list as the primary Data Structure */

/*Part A Structures*/

//Create the linked list
struct NODE
{
    char *val;
    struct NODE *next;
};
typedef struct NODE NODE;

//Hold the Pixel of the image
struct PIXEL
{
    int R;
    int G;
    int B;
};
typedef struct PIXEL PIXEL;

//Hold the PPM Image
struct PPM
{
    char format[2];;
    NODE *comments;
    int w, h;
    int max;
    PIXEL * arr;
};
typedef struct PPM PPM;


#if 0
The following function does not seem to actually perform a deep copy,
suggest looking carefully at the logic.
#endif

/**
* function used to deepcopy the linkedlist holding the comments.
* it returns a pointer to a deepcopy of the list passed as a parameter;
*/
//Copy the linked list
//Points to the copy
NODE *copy(NODE *first)
{
    NODE *second = NULL;
    NODE *previous = NULL;

    while (first != NULL)
    {
        NODE * temp = malloc(sizeof(NODE));
        if( !temp )
        {
            perror( "malloc failed" );
            // need logic here to pass all heap pointers to 'free()'
            exit( EXIT_FAILURE );
        }

        // implied else, malloc successful

        temp->val = first->val;
        temp->next = NULL;

        if (second == NULL)
        {
            second = temp;
            previous = temp;
        }

        else
        {
            previous->next = temp;
            previous = temp;
        }

        first = first->next;
    }
    return second;
} // end function: copy