Feedback from Code Review Stack Exchange - Reslashd/simple_encrypt GitHub Wiki

# Via Code Review Stack Exchange: https://codereview.stackexchange.com/questions/288139/simple-text-file-encryption-and-decryption

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:

return strcmp(file_content[lines_read - 1], "S_ENCRYPT23") == 0;

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.