Small one time pad encryption program

This one time pad encryption program I have written (basically just an XOR "encryption" program) seems to be working fine, compiling nicely (gcc -o ./OTP.c), and doing what it's supposed to. However I would like to improve it as much as possible which is why I am posting this.

I am particularly insecure about the memory allocation. Any suggestions regarding improvements are more than welcome!

The code in its entirety is found below and can also be found on Github: PrivacyProject/OTP-Encryption.

```
#include
#include
#include
#include

int main(int argc, char **argv)
{
struct stat statbuf;
struct stat keybuf;

char buffer [20];
int key;
int data;
int output;
int count;
char ans;
int * buf;
FILE * keyfile;
FILE * sourcefile;
FILE * destfile;

if(geteuid() !=0)
{
printf("Root access is required to run this program\n\n");
exit(0);
}

if(argc \n\n");
return (0);
}

/ Check number of arguments. /
if(argc>4)
{
printf("Too many arguments.\n");
printf("USAGE: OTP \n");
exit(1);
}

/ Allocate memory required by processes /
buf = (int*) malloc (sizeof(int));
if (buf == NULL)
{
perror("Error");
exit(1);
}

/ Lock down pages mapped to processes /
printf("Locking down processes...\n\n");
if(mlockall (MCL_CURRENT | MCL_FUTURE) \n");
exit (1);
}

/ Get size of sourcefile /
fstat(fileno(sourcefile), &statbuf);

/ Check if keyfile can be opened. /
if((keyfile = fopen(argv[3], "rb"))== NULL)
{
printf("Can't open keyfile.\n");
perror("Error");
printf("USAGE: OTP \n");
exit(1);
}

/ Get size of keyfile /
fstat(fileno(keyfile), &keybuf);

/ Check if keyfile is the same size as, or bigger than the sourcefile /
if((keybuf.st_size) < (statbuf.st_size))
{
printf("Source file is larger than keyfile.\n");
printf("This significantly reduces cryptographic strength.\n");
printf("Do you wish to continue? (Y/N)\n");
fgets(buffer, 20, stdin);
sscanf(buffer, "%c", &ans);
if(ans == 'n' || ans == 'N')
{
exit (1);
}
if(ans == 'y' || ans == 'Y')

Things you did well on:

-
You make good use of comments.

-
You try to make the user experience as smooth as possible, printing out a lot of useful information.

Things you could improve:

A few notes that others haven't covered:

-
Running your program through Valgrind, I didn't see any memory leaks besides where your if conditions fail and you exit main().

if((sourcefile = fopen(argv[1], "rb")) == NULL)
    {
        printf("Can't open source file\n");
        perror("Error");
        printf("USAGE: OTP   \n");
        exit(1);
    }

The majority of modern (and all major) operating systems will free memory not freed by the program when it ends. However, relying on this is bad practice and it is better to free it explicitly. Relying on the operating system also makes the code less portable.

if((sourcefile = fopen(argv[1], "rb")) == NULL)
{
    puts("Can't open source file.");
    perror("Error");
    free(buf)
    return -5;
}

-
Your encryption method is secure, banking on the fact that the all of the conditions for your program are met.

But I always think worst case scenario, where the user doesn't follow your program's optimal conditions. If you want to use secure encryption techniques, you can benefit from a cryptography expert's work. This means you will have to implement an external library (such as OpenSSL). Here is an example if you want to go that route.

-
You have an implicit declaration of function geteuid(), which is invalid in the C99 standard.

#include 

-
fopen(), a widely-used file I/O functions that you are using, got a facelift in C11. It now supports a new exclusive create-and-open mode (“...x“). The new mode behaves like O_CREAT|O_EXCL in POSIX and is commonly used for lock files. The “...x” family of modes includes the following options:

-
wx create text file for writing with exclusive access.

-
wbx create binary file for writing with exclusive access.

-
w+x create text file for update with exclusive access.

-
w+bx or wb+x create binary file for update with exclusive access.

Opening a file with any of the exclusive modes above fails if the file already exists or cannot be created. Otherwise, the file is created with exclusive (non-shared) access. Additionally, a safer version of fopen() called fopen_s() is also available. That is what I would use in your code if I were you, but I'll leave that up for you to decide and change.

-
You can cut down on a few lines of code by initializing similar types on one line. This will keep you more organized.

struct stat statbuf;
   struct stat keybuf;

   char buffer [20];
   int key;
   int data;
   int output;
   int count;
   char ans;
   int * buf;
   FILE * keyfile;
   FILE * sourcefile;
   FILE * destfile;

Also, initialize your int values when you declare them. Pointer values will default to NULL.

struct stat statbuf, keybuf;

char buffer [20];
char ans;
int key = 0, data = 0, output = 0, count = 0;
int *buf;
FILE *keyfile, *sourcefile, *destfile;

-
You check if argc is greater or less than 4.

if(argc4)
{
    printf("Too many arguments.\n");
    printf("USAGE: OTP   \n");
    exit(1);
}

Just check for the inequality of 4 and print the block statement.

if (argc != 4)
{
    // printf() block
    return 0;
}

-
I would have extracted all of the encryption to one method and all of the file validation in another method, but I'll leave that for you to implement.

-
You can remove the != 0 for maximum C-ness, but this is to your discretion and tastes.

if(geteuid() != 0)

-
It is more common to return 0; rather than to exit(0). Both will call the registered atexit handlers and will cause program termination though. You have both spread throughout your code. Choose one to be consistent. Also, you should return different values for different errors, so you can pinpoint where something goes wrong in the future.

-
Use puts() instead of printf() with a bunch of '\n' characters.

printf("    and  generates an output file with the resulting\n");

puts("    and generates an output file with the resulting");

-
You compare some pointers to NULL in some test conditions.

if (buf == NULL)

You can simplify them.

if (!buf)

-
Your perror() messages could be more descriptive.

-
You compare some input to the uppercase and lowercase versions of characters.

if (ans == 'n' || ans == 'N')

You could use the tolower() function in ` to simplify it a bit.

if (tolower(ans) == 'n')

-
Already mentioned, but please indent your code properly. It will make your code a lot easier to read and maintain. You can find IDE's out there that will do it automatically for you when told.

Final code (with my changes implemented):

``
#include
#include
#include
#include
#include
#include

int main(int argc, char **argv)
{
struct stat statbuf, keybuf;

cha

StackExchange Code Review Q#41748, answer score: 16