1
\$\begingroup\$

Short introduction; recently I've completed a basic course on C but other than that I have no programming experience and I am a total beginner.

As a practice project and to practice reading from and writing to files I've created this simple text encryption program in C.

It basically asks the user for a text file to open (but no file extension check on that yet) and gives the user the choice to either encrypt or decrypt the file based on if it already has been encrypted before by checking if the signature is present at the last line of the file or not.

Would love some general feedback on anything I could improve on (program structure, style, error handling, security, use of functions, whatever else comes to mind) but also what things I should keep on doing if there are any!

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

#define MAX_LINES 1024
#define MAX_LENGTH 1024
#define FILENAME_LENGTH 100
#define WRITE_MODE "w"
#define READ_MODE "r"    

void addSignature(char file_content[][MAX_LENGTH], int lines_read);
int checkSignature(char file_content[][MAX_LENGTH], int lines_read);
void closeFile(FILE * file_ptr);
void encryptData(char file_content[][MAX_LENGTH], int lines_read);
void decryptData(char file_content[][MAX_LENGTH], int lines_read);
void getFilename(char* file_name);
FILE* openFile(char* file_name, char* file_mode);
int readFile(char file_content[][MAX_LENGTH], FILE * file_ptr);
void removeSignature(char file_content[][MAX_LENGTH], int lines_read);
void showMenu(int has_signature, char file_content[][MAX_LENGTH], int lines_read, FILE * file_ptr, char* file_name);
void writeFile(char file_content[][MAX_LENGTH], FILE * file_ptr, int lines_read);

int main(void)
{
    FILE *file_ptr = NULL;
    char file_name[FILENAME_LENGTH] = {'\0'};       
    char file_content[MAX_LINES][MAX_LENGTH] = {'\0'};
    int lines_read = 0;
    int has_signature = 0;
    
    
    printf("*** SIMPLE ENCRYPT 1.0 by RESLASHD ***\n");
    getFilename(file_name); 
    printf("\nAttempting to open file %s ...\n", file_name);
    
    if (( file_ptr = openFile(file_name, READ_MODE)) != NULL){
        printf("\n*** FILE OPENED ***\n");
        
        // Read file and store in array
        lines_read = readFile(file_content, file_ptr);  
        closeFile(file_ptr);
    
        // Set signature flag for showMenu
        has_signature = checkSignature(file_content, lines_read);
    
        showMenu(has_signature, file_content, lines_read, file_ptr, file_name);     
    }
    printf("\n*** EXITING PROGRAM ***\n");
    return 0;
}

void addSignature(char file_content[][MAX_LENGTH], int lines_read){
    strcpy(file_content[lines_read], "S_ENCRYPT23");
}

int checkSignature(char file_content[][MAX_LENGTH], int lines_read){
    if(strcmp(file_content[lines_read-1],"S_ENCRYPT23") == 0){
        // File is encrypted
        return 1;
    } else{
        // File is not encrypted
        return 0;
    }       
}

void closeFile(FILE * file_ptr)
{   
    // NULL means no file is open so nothing to close
    if ( file_ptr != NULL ){
        fclose(file_ptr);
    }
}

void encryptData(char file_content[][MAX_LENGTH], int lines_read){
    int row = 0; // line number (0 is the first line)
    int column = 0; // character number (0 is the first character on a line)
    
    for(row = 0 ; row < lines_read ; row++){
        column = 0;
        while(file_content[row][column] != '\0'){
            if(file_content[row][column] != '\n'){
                file_content[row][column] = file_content[row][column]+1;
                column++;
            } else{
                column++;
            }       
        }
    }
    addSignature(file_content, lines_read);
}

void decryptData(char file_content[][MAX_LENGTH], int lines_read){
    int row = 0; // line number (0 is the first line)
    int column = 0; // character number (0 is the first character on a line)
    lines_read = lines_read - 1;
    for(row = 0 ; row <= lines_read ; row++){
        column = 0;
        while(file_content[row][column] != '\0'){
            if(file_content[row][column] != '\n'){
                file_content[row][column] = file_content[row][column]-1;
                column++;
            } else{
                column++;
            }       
        }
    }
    lines_read = lines_read + 1;
    removeSignature(file_content, lines_read);
}

void getFilename(char* file_name){
    
    printf("\nPlease enter the name of the file you want encrypt/decrypt  > ");
    fgets(file_name, 100, stdin);       
    file_name[strcspn(file_name, "\n")] = 0;    
}

FILE* openFile(char* file_name, char* file_mode)
{
    FILE *file_ptr = NULL;
    if (( file_ptr = fopen(file_name, file_mode )) == NULL ){
        printf("\n*** ERROR OPENING FILE ***\n");
    }
    return file_ptr;
}

int readFile(char file_content[][MAX_LENGTH], FILE * file_ptr){
    int lines_read = 0;
    
    // While the end of the file is not reached yet AND there are no errors reading the file
    while (!feof(file_ptr) && !ferror(file_ptr)){
        if (fgets(file_content[lines_read], MAX_LENGTH, file_ptr) != NULL){
            lines_read++;   
        }
     }
    return lines_read;
}

void removeSignature(char file_content[][MAX_LENGTH], int lines_read){
    strcpy(file_content[lines_read-1], "\0");
}

void showMenu(int has_signature, char file_content[][MAX_LENGTH], int lines_read, FILE * file_ptr, char* file_name){
    char choice = '\0'; 
    
    printf("\n********* MENU *********\n");
    if( has_signature == 0) {
        printf("1 = Encrypt File \n");  
    }else{
        printf("1 = Decrypt File \n");  
    }
    printf("2 = Close file and exit\n\n");
    printf("************************\n");
    choice = getchar();
    
    if( has_signature == 0 && choice == '1'){
        printf("\nEncrypting file...\n");
        //Open file for writing (erasing all contents!)
        openFile(file_name, WRITE_MODE);        
        encryptData(file_content, lines_read);  
        writeFile(file_content, file_ptr, lines_read);
        closeFile(file_ptr);        
        printf("\n*** FILE ENCRYPTED ***\n");   
    } else if(has_signature == 1 && choice == '1'){
        printf("\nDecrypting file...\n");
        //Open file for writing (erasing all contents!)
        openFile(file_name, WRITE_MODE);        
        decryptData(file_content, lines_read);
        writeFile(file_content, file_ptr, lines_read);
        closeFile(file_ptr);    
        printf("\n*** FILE DECRYPTED ***\n");   
    }   
    else{
        printf("\n*** NO CHANGES MADE TO FILE ***\n");      
    }
}

void writeFile(char file_content[][MAX_LENGTH], FILE * file_ptr, int lines_read){
    int row = 0; // line number (0 is the first line)
    int column = 0; // character number (0 is the first character on a line)
    lines_read = lines_read + 1;
    
    for(row = 0 ; row < lines_read ; row++){
        
        column = 0;
        while(file_content[row][column] != '\0'){   
            fprintf(file_ptr, "%c", file_content[row][column]);         
            column++;
        }       
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Its kinda like a basic 'Caesar Cipher' where each read char is shifted by 1. \$\endgroup\$
    – Reslashd
    Nov 29, 2023 at 6:46

1 Answer 1

1
\$\begingroup\$
  • Enable more compiler warnings.
$ gcc -std=c17 -fPIC -gdwarf-4 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wconversion  -Wstrict-prototypes -fanalyzer encrypt.c -o enc
encrypt.c: In function 'main':
encrypt.c:9:19: warning: passing argument 2 of 'openFile' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    9 | #define READ_MODE "r"
      |                   ^~~
encrypt.c:36:42: note: in expansion of macro 'READ_MODE'
   36 |     if (( file_ptr = openFile(file_name, READ_MODE)) != NULL){
      |                                          ^~~~~~~~~
encrypt.c:17:39: note: expected 'char *' but argument is of type 'const char *'
   17 | FILE* openFile(char* file_name, char* file_mode);
      |                                 ~~~~~~^~~~~~~~~
encrypt.c: In function 'showMenu':
encrypt.c:155:14: warning: conversion from 'int' to 'char' may change value [-Wconversion]
  155 |     choice = getchar();
      |              ^~~~~~~
encrypt.c:8:20: warning: passing argument 2 of 'openFile' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    8 | #define WRITE_MODE "w"
      |                    ^~~
encrypt.c:160:29: note: in expansion of macro 'WRITE_MODE'
  160 |         openFile(file_name, WRITE_MODE);
      |                             ^~~~~~~~~~
encrypt.c:119:39: note: expected 'char *' but argument is of type 'const char *'
  119 | FILE* openFile(char* file_name, char* file_mode)
      |                                 ~~~~~~^~~~~~~~~
encrypt.c:8:20: warning: passing argument 2 of 'openFile' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    8 | #define WRITE_MODE "w"
      |                    ^~~
encrypt.c:168:29: note: in expansion of macro 'WRITE_MODE'
  168 |         openFile(file_name, WRITE_MODE);
      |                             ^~~~~~~~~~
encrypt.c:119:39: note: expected 'char *' but argument is of type 'const char *'
  119 | FILE* openFile(char* file_name, char* file_mode)
      |                                 ~~~~~~^~~~~~~~~
encrypt.c: In function 'openFile':
encrypt.c:125:12: warning: leak of FILE 'file_ptr' [CWE-775] [-Wanalyzer-file-leak]
  125 |     return file_ptr;
      |            ^~~~~~~~
  'showMenu': events 1-4
    |
    |  144 | void showMenu(int has_signature, char file_content[][MAX_LENGTH], int lines_read, FILE * file_ptr, char* file_name){
    |      |      ^~~~~~~~
    |      |      |
    |      |      (1) entry to 'showMenu'
    |......
    |  157 |     if( has_signature == 0 && choice == '1'){
    |      |       ~
    |      |       |
    |      |       (2) following 'true' branch...
    |  158 |         printf("\nEncrypting file...\n");
    |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (3) ...to here
    |  159 |         //Open file for writing (erasing all contents!)
    |  160 |         openFile(file_name, WRITE_MODE);
    |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |         |
    |      |         (4) calling 'openFile' from 'showMenu'
    |
    +--> 'openFile': events 5-10
           |
           |  119 | FILE* openFile(char* file_name, char* file_mode)
           |      |       ^~~~~~~~
           |      |       |
           |      |       (5) entry to 'openFile'
           |......
           |  122 |     if (( file_ptr = fopen(file_name, file_mode )) == NULL ){
           |      |        ~             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |        |             |
           |      |        |             (6) opened here
           |      |        (7) assuming 'file_ptr' is non-NULL
           |      |        (8) following 'false' branch (when 'file_ptr' is non-NULL)...
           |......
           |  125 |     return file_ptr;
           |      |            ~~~~~~~~
           |      |            |
           |      |            (9) ...to here
           |      |            (10) 'file_ptr' leaks here; was opened at (6)
           |

  • WRITE_MODE and READ_MODE are unnecessary. "w" and "r" are clear as they are.

  • You won't require the forward declarations if you simply define move the definition of main() to the end of the program.

  • Use puts()/fputs() when the formatting capabilities of printf()/fprintf() are not required.

  • Reduce scope of variables. In main(), lines_read and has_signature need not be visible outside of the (only) if statement.

  • Use bool from stdbool.h to denote a binary state instead of 0 and 1. has_signature should be a bool.

  • Use size_t for sizes, cardinalities, and ordinal numbers. lines_read need not be signed.

  • Use EXIT_SUCCESS and EXIT_FAILURE from stdlib.h as the exit status codes instead of magic values.

  • openFile() simply returns NULL if fopen() fails. I don't see a point of writing a wrapper around fopen() here, especially when you ignore the return value of that wrapper.

  • closeFile() won't be required if you check the return value of fopen(), or the wrapper around it.

  • readFile() doesn't check whether there are more than MAX_LINES in the file and would continue to write past the buffer in case it does, which would result in undefined behavior. But why is there a limit to the number and length of lines? What if someone wants to encrypt a file that has more than MAX_LINES lines, or lines exceeding MAX_LENGTH characters? Dynamic memory allocation would be more apt here.

  • addSignature() doesn't check whether lines_read < MAX_LINES before copying the signature. Same for checkSignature() and removeSignature().

  • checkSignature() can be simplified:

    int checkSignature(char file_content[][MAX_LENGTH], int lines_read) {
    #if 0
        if(strcmp(file_content[lines_read-1],"S_ENCRYPT23") == 0){
            // File is encrypted
            return 1;
        } else{
            // File is not encrypted
            return 0;
        }
    #else
        return strcmp(file_content[lines_read - 1], "S_ENCRYPT23") == 0;
    #endif
    }
    
  • In getFileName(), the return value of fgets() is ignored.

  • lines_read = lines_read + 1; can be written as: lines_read += 1, or ++lines_read/lines_read++. Though, I don't see why you decremented lines_read by 1 before looping through the lines only to choose i <= lines_read as the loop condition, and then incrementing it back to the previous value.

  • encryptData() and decryptData() are basically the same function. Consider:

    void crypt(char file_content[][MAX_LENGTH], int lines_read, int op) {
       int row = 0; // line number (0 is the first line)
       int column = 0; // character number (0 is the first character on a line)
    
       for (row = 0; row < lines_read; row++) {
            column = 0;
            while (file_content[row][column] != '\0') {
                if (file_content[row][column] != '\n') {
                    file_content[row][column] = (op == 1) ? file_content[row][column] + 1 : file_content[row][column] - 1;
                    column++;
                } else {
                    column++;
                }
            }
        }
    }
    
    void encryptData(char file_content[][MAX_LENGTH], int lines_read) {
       crypt(file_content, lines_read, 1);
       addSignature(file_content, lines_read);
    }
    
    void decryptData(char file_content[][MAX_LENGTH], int lines_read) {
        crypt(file_content, lines_read, 0);
        removeSignature(file_content, lines_read);
    } 
    
  • It would have been better if the program accepted the file name and the operation to be performed as command-line arguments.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you for your in depth analysis and recommendations! I do have 2 questions; you mention I return magic exit status codes but I'm not sure what you mean by that? Could you give an example in the code? The other question is about the forward declarations; I know it works without those if I move main() to the end of the program but is this something I should really do? Is (not forward declaring functions) and moving main() to the end of the program good programming practice? Or this a matter of preferred coding style? What are the pro's and cons? \$\endgroup\$
    – Reslashd
    Nov 30, 2023 at 19:22
  • \$\begingroup\$ See: stackoverflow.com/a/8868139/20017547 \$\endgroup\$
    – Harith
    Dec 1, 2023 at 1:43
  • \$\begingroup\$ And: stackoverflow.com/a/58994770/20017547 \$\endgroup\$
    – Harith
    Dec 1, 2023 at 1:50
  • \$\begingroup\$ Third question: if I use size_t for lines_read I assume I should use that for the row & column variables as well? (To prevent compiler warnings). Should size_t always be used in a for loop instead of int assuming the iteration gives no negative values? \$\endgroup\$
    – Reslashd
    Dec 5, 2023 at 16:16
  • \$\begingroup\$ Yes. See: stackoverflow.com/q/2550774/20017547 \$\endgroup\$
    – Harith
    Dec 5, 2023 at 17:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.