8
\$\begingroup\$

I was learning about getopt to parse command line options and I decided to write this small XOR cipher program for files.

It acts as a "UNIX filter" (I guess) since default input and output are stdin and stdout. It can generate a random key file from /dev/urandom if the provided one doesn't exists (size of this file can also be specified).

I split the thing in three files and here they are:

fxor.h

#ifndef FXOR_HEADER
#define FXOR_HEADER

#include <stdio.h>
#include <stdbool.h>

#define URANDOM "/dev/urandom"

/*
 * fxor:  applies XOR cipher to bytes from in file using bytes from key file as
 *        key, writes result to out file and returns true.
 *        If key file is smaller than in file, key file will restart from
 *        beginning each time EOF is reached.
 *
 *        in and key files are assumed to be opened in read mode, out file in
 *        write mode. in, out, key can't be the same file, otherwise behaviour
 *        is undefined.
 *
 *        If an error occurs while writing to out, returns false.
 */
bool fxor(FILE *in, FILE *out, FILE *key);

/*
 * fwrite_urandom:  writes nbytes bytes to out file from URANDOM and returns
 *                  true. out file is assumed to be opened in write mode.
 *                  If URANDOM can't be opened or an error occurs while writing
 *                  to out, returns false.
 */
bool fwrite_urandom(FILE *out, size_t nbytes);

#endif

fxor.c

#include "fxor.h"


bool fxor(FILE *in, FILE *out, FILE *key)
{
        unsigned char in_byte;
        unsigned char key_byte;
        int c;
        while ((c = fgetc(in)) != EOF) {
                in_byte = c;

                if ((c = fgetc(key)) == EOF) {
                        rewind(key);
                        c = fgetc(key);
                }
                key_byte = c;

                if (fputc(in_byte ^ key_byte, out) == EOF) {
                        return false;
                }
        }
        return true;
}

bool fwrite_urandom(FILE *out, size_t nbytes)
{
        FILE *urandom;
        if ((urandom = fopen(URANDOM, "rb")) == NULL) {
                return false;
        }
        while (nbytes-- > 0) {
                if (fputc(fgetc(urandom), out) == EOF) {
                        fclose(urandom);
                        return false;
                }
        }
        fclose(urandom);
        return true;
}

main.c

#include <stdio.h>
#include <errno.h>
#include <getopt.h>
#include <ctype.h>
#include <stdint.h>

#include "fxor.h"

#define DEFAULT_KEY_LENGTH 65536

/*
 * usage:  print an usage message.
 */
static void usage(const char *prog_name);

/*
 * atost:  convert str to a size_t value and returns it.
 *         No check for overflow is made, if str contains an invalid size_t
 *         value, SIZE_MAX will be returned. str can start with '+'. If str is
 *         an empty string 0 will be returned.
 */
static size_t atost(const char *str);

/*
 * open_or_gen_key:  if key_filename exists, it will be opened in read mode and
 *                   the pointer returned, otherwise, if it doesn't exists, it
 *                   will be created, its size will be key_length bytes and it
 *                   will be filled with bytes from URANDOM. A pointer to it
 *                   will be returned after reopening the file in read mode.
 *                   If an error occurs, returns NULL.
 */
static FILE *open_or_gen_key(const char *key_filename, size_t key_length);

/*
 * fclose_all:  close three file it gets as input. It is safe to pass NULL
 *              pointer as file, no operation on it will be performed.
 */
static void fclose_all(FILE *in, FILE *out, FILE *key);


static void usage(const char *prog_name)
{
        printf("usage: %s -k key [-l key_length] [-i input] [-o output] [-h]\n\n"
               "  -k key         key filename, required. If doesn't exists, a file\n"
               "                 filled with bytes from %s will be created and\n"
               "                 used for xor operation, default size will be %d bytes,\n"
               "                 you can specify a different one with -l option.\n\n"
               "  -l key_length  optional size in bytes for key file if has to be generated,\n"
               "                 must be >= 0. If -k file exists, -l will not be used.\n\n"
               "  -i input       input file, default standard input.\n\n"
               "  -o output      output file, default standard output.\n\n"
               "  -h             show this message and exit.\n",
               prog_name, URANDOM, DEFAULT_KEY_LENGTH);
}

static size_t atost(const char *str)
{
        size_t value = 0;
        for (size_t i = (str[0] == '+') ? 1 : 0;
             str[i] != '\0';
             i++) {
                if ( ! isdigit(str[i])) {
                        return SIZE_MAX;
                }
                value = value * 10 + (str[i] - '0');
        }
        return value;
}

static FILE *open_or_gen_key(const char *key_filename, size_t key_length)
{
        FILE *key;
        if ((key = fopen(key_filename, "rb")) == NULL && errno == ENOENT) {
                if ((key = fopen(key_filename, "wb")) == NULL) {
                        return NULL;
                }
                if ( ! fwrite_urandom(key, key_length)) {
                        fclose(key);
                        return NULL;
                }
                key = freopen(key_filename, "rb", key);
        }
        return key;
}

static void fclose_all(FILE *in, FILE *out, FILE *key)
{
        if (in != NULL) {
                fclose(in);
        }
        if (out != NULL) {
                fclose(out);
        }
        if (key != NULL) {
                fclose(key);
        }
}

int main(int argc, char **argv)
{
        char *in_filename, *out_filename, *key_filename, *key_length_str;
        in_filename = out_filename = key_filename = key_length_str = NULL;

        int o;
        while ((o = getopt(argc, argv, "k:l:i:o:h")) != -1) {
                switch (o) {
                case 'k':
                        key_filename = optarg;
                        break;
                case 'l':
                        key_length_str = optarg;
                        break;
                case 'i':
                        in_filename = optarg;
                        break;
                case 'o':
                        out_filename = optarg;
                        break;
                case 'h':
                        usage(argv[0]);
                        return 0;
                default:
                        return 1;
                }
        }
        if (key_filename == NULL) {
                fprintf(stderr, "%s: option is required -- 'k'\n", argv[0]);
                return 1;
        }

        FILE *in = stdin;
        FILE *out = stdout;
        FILE *key = NULL;
        size_t key_length = DEFAULT_KEY_LENGTH;

        if (key_length_str != NULL && (key_length = atost(key_length_str)) == SIZE_MAX) {
                fprintf(stderr, "%s: invalid argument for option -- 'l'\n", argv[0]);
                return 1;
        }
        if (in_filename != NULL && (in = fopen(in_filename, "rb")) == NULL) {
                perror(in_filename);
                return 1;
        }
        if (out_filename != NULL && (out = fopen(out_filename, "wb")) == NULL) {
                perror(out_filename);
                fclose_all(in, out, key);
                return 1;
        }
        if ((key = open_or_gen_key(key_filename, key_length)) == NULL) {
                perror(key_filename);
                fclose_all(in, out, key);
                return 1;
        }

        if ( ! fxor(in, out, key)) {
                fprintf(stderr, "%s: error while applying xor\n", argv[0]);
                fclose_all(in, out, key);
                return 1;
        }

        fclose_all(in, out, key);
}

Example:

$ cc -std=c11 -Wall -Wextra -Werror -o fxor fxor.c main.c 
$ l
fxor*  fxor.c  fxor.h  main.c
$ echo "Hello fxor!" | ./fxor -k hello.key -l 16 -o hello.xor
$ ll | grep "hello*"
-rw-r--r-- 1 marco marco    16 Nov 18 16:16 hello.key
-rw-r--r-- 1 marco marco    12 Nov 18 16:16 hello.xor
$ ./fxor -i hello.xor -k hello.key 
Hello fxor!
$ xxd -b hello.key 
00000000: 10000010 01110111 01110010 01110001 01110100 10011111  .wrqt.
00000006: 00111111 10110001 10110001 10011011 10111010 00111011  ?....;
0000000c: 01110010 01001000 11100110 11001111                    rH..
$ xxd -b hello.xor
00000000: 11001010 00010010 00011110 00011101 00011011 10111111  ......
00000006: 01011001 11001001 11011110 11101001 10011011 00110001  Y....1
$ ./fxor -i hello.xor -k hello.key | xxd -b
00000000: 01001000 01100101 01101100 01101100 01101111 00100000  Hello 
00000006: 01100110 01111000 01101111 01110010 00100001 00001010  fxor!.
$

I'd like to get some feedback about style and advice about dealing with files and errors: how can I improve the "if cascade" made of "if fopen fails, print an error, close things opened before and return 1"?

Also, I bet that opening a device just to get random bytes isn't the best thing to do. Do you have any suggestion on that?

\$\endgroup\$
5
\$\begingroup\$

First, nice question. The code is well formatted and easy to read. I do like the block comments preceding most of the functions, although usage may be self explanatory.

MACROS
If the compiler is compliant to the C89 standard or the C99 standard then macros are the preferred way to define constants. This may have changed in the C11 standard. Some people confuse this portion of C with C++, and in C++ it is better to use static const or static constexpr.

Variable Names
The variable name o is a little terse, option might be a better name. I agree with another answer that the variables in and out might be better named in_file and out_file.

When to Use Function Prototypes
It is not always necessary to use function prototypes, in the main.c file n this question they aren't necessary. If the main() function was the first function in the file it would be necessary, but since all the functions used by main are previously defined it is not necessary in this case. The compiler will let you know when it is necessary. In the slightly modified code below I have removed the function prototypes and this compiles without errors or warnings.

Making the Code More Readable
The standard include file stdlib.h contains 2 macros that might make the code a little more readable. These macros are EXIT_SUCCESS and EXIT_FAILURE. The code below has been modified to show the use of these macros.

Put variable Assignments on Separate Lines
When maintaining existing code the following two lines may be error prone:

    char *in_filename, *out_filename, *key_filename, *key_length_str;
    in_filename = out_filename = key_filename = key_length_str = NULL;

It might be better to have a separate line for each one:

    char *in_filename = NULL;
    char *out_filename = NULL;
    char *key_filename = NULL;
    char *key_length_str = NULL;

That way if one of the variables needs to be initialized with a different value it is easier to find and implement.

Simplify main()
It might be better to create 2 more functions that precede main(), one that parses the command line and a second that opens all the files. Both functions could return EXIT_FAILURE or EXIT_SUCCESS as necessary. Generally the main function should be as simple as possible and all other logic is in other functions.

Slightly Modified Code

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <getopt.h>
#include <ctype.h>
#include <stdint.h>

#include "fxor.h"

#define DEFAULT_KEY_LENGTH 65536

/*
 * usage:  print an usage message.
 */
static void usage(const char *prog_name)
{
    printf("usage: %s -k key [-l key_length] [-i input] [-o output] [-h]\n\n"
           "  -k key         key filename, required. If doesn't exists, a file\n"
           "                 filled with bytes from %s will be created and\n"
           "                 used for xor operation, default size will be %d bytes,\n"
           "                 you can specify a different one with -l option.\n\n"
           "  -l key_length  optional size in bytes for key file if has to be generated,\n"
           "                 must be >= 0. If -k file exists, -l will not be used.\n\n"
           "  -i input       input file, default standard input.\n\n"
           "  -o output      output file, default standard output.\n\n"
           "  -h             show this message and exit.\n",
           prog_name, URANDOM, DEFAULT_KEY_LENGTH);
}

/*
 * atost:  convert str to a size_t value and returns it.
 *         No check for overflow is made, if str contains an invalid size_t
 *         value, SIZE_MAX will be returned. str can start with '+'. If str is
 *         an empty string 0 will be returned.
 */
static size_t atost(const char *str)
{
    size_t value = 0;
    for (size_t i = (str[0] == '+') ? 1 : 0;
         str[i] != '\0';
         i++) {
        if ( ! isdigit(str[i])) {
            return SIZE_MAX;
        }
        value = value * 10 + (str[i] - '0');
    }
    return value;
}

/*
 * open_or_gen_key:  if key_filename exists, it will be opened in read mode and
 *                   the pointer returned, otherwise, if it doesn't exists, it
 *                   will be created, its size will be key_length bytes and it
 *                   will be filled with bytes from URANDOM. A pointer to it
 *                   will be returned after reopening the file in read mode.
 *                   If an error occurs, returns NULL.
 */
static FILE *open_or_gen_key(const char *key_filename, size_t key_length)
{
    FILE *key;
    if ((key = fopen(key_filename, "rb")) == NULL && errno == ENOENT) {
        if ((key = fopen(key_filename, "wb")) == NULL) {
            return NULL;
        }
        if ( ! fwrite_urandom(key, key_length)) {
            fclose(key);
            return NULL;
        }
        key = freopen(key_filename, "rb", key);
    }
    return key;
}

/*
 * fclose_all:  close three file it gets as input. It is safe to pass NULL
 *              pointer as file, no operation on it will be performed.
 */
static void fclose_all(FILE *in, FILE *out, FILE *key)
{
    if (in != NULL) {
        fclose(in);
    }
    if (out != NULL) {
        fclose(out);
    }
    if (key != NULL) {
        fclose(key);
    }
}

int main(int argc, char **argv)
{
    char *in_filename, *out_filename, *key_filename, *key_length_str;
    in_filename = out_filename = key_filename = key_length_str = NULL;

    int o;
    while ((o = getopt(argc, argv, "k:l:i:o:h")) != -1) {
        switch (o) {
            case 'k':
                key_filename = optarg;
                break;
            case 'l':
                key_length_str = optarg;
                break;
            case 'i':
                in_filename = optarg;
                break;
            case 'o':
                out_filename = optarg;
                break;
            case 'h':
                usage(argv[0]);
                return EXIT_SUCCESS;

            default:
                return EXIT_FAILURE;
        }
    }
    if (key_filename == NULL) {
        fprintf(stderr, "%s: option is required -- 'k'\n", argv[0]);
        return EXIT_FAILURE;
    }

    FILE *in = stdin;
    FILE *out = stdout;
    FILE *key = NULL;
    size_t key_length = DEFAULT_KEY_LENGTH;

    if (key_length_str != NULL && (key_length = atost(key_length_str)) == SIZE_MAX) {
        fprintf(stderr, "%s: invalid argument for option -- 'l'\n", argv[0]);
        return EXIT_FAILURE;
    }
    if (in_filename != NULL && (in = fopen(in_filename, "rb")) == NULL) {
        perror(in_filename);
        return EXIT_FAILURE;
    }
    if (out_filename != NULL && (out = fopen(out_filename, "wb")) == NULL) {
        perror(out_filename);
        fclose_all(in, out, key);
        return EXIT_FAILURE;
    }
    if ((key = open_or_gen_key(key_filename, key_length)) == NULL) {
        perror(key_filename);
        fclose_all(in, out, key);
        return EXIT_FAILURE;
    }

    if ( ! fxor(in, out, key)) {
        fprintf(stderr, "%s: error while applying xor\n", argv[0]);
        fclose_all(in, out, key);
        return EXIT_FAILURE;
    }

    fclose_all(in, out, key);

    return EXIT_SUCCESS;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you! I'll remove prototypes inside main.c and I'll use EXIT_* macros from now on. \$\endgroup\$ – MarcoLucidi Nov 20 '17 at 18:25
3
\$\begingroup\$

Overall, this is really well written. It's very easy to read and very idiomatic. I see a few really minor things that I would do differently.

Don't Be Too Terse

I see a lot of cases where you've done something like this:

if ((x = somefunction()) == somevalue) {
    doSomething();
}

I don't see a lot of value in cramming everything onto a single line like that. When possible, I prefer to put the assignment before the if:

x = somefunction();
if (x == somevalue) {
    doSomething();
}

It's easier to read that way. It can't be avoided in the while case without repeating the assignment, so it's a trade-off that I'm OK with in that case.

Naming

For the most part your names are great. I would be careful when you have a situation like you do with fxor(). You have an argument named in and one named out. That makes it seem like one is an input variable and one is an output variable. I would rename them input_file and output_file to avoid that confusion.

Macros vs. Constants

You've used a couple of macros in this code. They are completely safe the way you've done them, so no complaints there. (In case you aren't aware, macros that take arguments can have some nasty side effects that can lead to bugs.) For the DEFAULT_KEY_LENGTH, I'd prefer to see it declared as a constant, like this:

static const size_t DEFAULT_KEY_LENGTH = 65536;

The reason being that it allows the compiler to do proper type checking. If, for example, someone attempted to assign it to a char or a signed short, you're more likely to get an appropriate warning from the compiler if it has a proper type.

For URANDOM, what's the purpose of making it a #define? I see that on macOS they have /dev/random/ and because Linux uses it, they include /dev/urandom/ for compatibility's sake. Perhaps some systems only use one or the other? I'd look into whether there's a better way to figure out what to use. (It may be that there's no way to do that at compile time - I don't know.) That's getting a little off into the weeds of build environments rather than code, so I'll just suggest looking into it. What you have might be the only way to do it.

Don't Reinvent the Wheel

What's the point of writing atost()? You have several functions already available to you like atol(), atoll(), strtol(), sscanf(), etc. Given that on many systems size_t is 64-bits, and this is going to be the length of a file, I don't think you need this function. Is anyone really going to create a key file that's over 4 GBs? Should they? Even if you do want to, you can use atoll(), so I'd remove this function and use the standard one.

Use More Functions

You've done a pretty good job of breaking things into functions. I think you could do even more. I'd move the argument parsing loop into its own function. I'd also move the file opening and checking into its own function.

Cleaning Up

You asked about cleaning up the 5 if statements at the end. I don't think it's that bad at all, and if you put the first 4 into a separate function, it would only be 2 if statements - 1 to test the result of the function that opens the files, and 1 to call fxor(). But there are other ways you could handle it.

You could group the key handling together. I see why you did it the way you did. If the key_length_str is empty or the length is too large, you don't want to try to open any of the files, and you don't want to generate a key and save it when you might need to end up deleting it if you can't open one of the other files. I wouldn't worry too much about opening the input and output files. Just opening them isn't a performance issue. So you could do something like this:

int open_file(const char* filename, FILE** file, const char* mode)
{
    int result = 0;
    if (filename != NULL) {
        *file = fopen(filename, mode);
        if (*file == NULL) {
            perror(filename):
            result = 1;
        }
    }
    return result;
}

int open_files (const char* input_filename, FILE** input_file, const char* output_filename, FILE** output_file)
{
    int result = open_file(input_filename, input_file, "rb");

    if (result == 0) {
        result = open_file(output_filename, output_file, "wb");
    }

    if (result != 0) {
        flose_all(*input_file, *output_file, NULL);
    }

    return result;
}

size_t get_key_length(const char* key_length_str)
{
    size_t key_length = 0;
    if (key_length_str != NULL) {
        key_length = atoll(key_length_str);
    }
}

Then in main() (or some function which handles opening and setting up all the files), you could do this:

if (open_files (in_filename, &in, out_filename, &out) == 0) {
    key_length = get_key_length(key_length_str);
    if (key_length == SIZE_MAX) {
        fprintf(stderr, "%s: invalid argument for option -- 'l'\n", argv[0]);
        return 1;
    }
}

key = open_or_gen_key(key_filename, key_length);
if (key == NULL) {
    perror(key_filename);
    fclose (key);
    return 1;
}

As I said at the beginning, these are really minor things. This is already in really good shape. Nice work!

\$\endgroup\$
  • \$\begingroup\$ Thank you! I removed atost and I'm looking into atol family. Probably I'm going to remove URANDOM define too, you're right, it's useless and finally I'll wrap getopt loop and files setup into two separate functions. \$\endgroup\$ – MarcoLucidi Nov 20 '17 at 18:33
  • \$\begingroup\$ atost() attempts to detect negative and invalid chracters and reports with an "error" value. size_t key_length = atoll(...) does not detect those problems. \$\endgroup\$ – chux Nov 22 '17 at 17:45
  • \$\begingroup\$ That's a fair point. I guess my main concern was the shortcomings you pointed out in your review. I think you covered it nicely. \$\endgroup\$ – user1118321 Nov 22 '17 at 19:32
3
\$\begingroup\$

atost() review.

Providing the wrapper function atost() is a good idea to detect invalid input, but the implementation has shortcomings.


Certainly atost() is meant to fail with negative numbers, so direct application of atoi() family of functions will not work the same.

Another difference between atost() and ato*()/strto*() is that atost() rejects strings that begin with white-space. Based on codes usage of atost(), the seems to be an unimportant difference.

The design of atost() is weak. It can "fail" at least 4 different ways. It returns 0 or SIZE_MAX on 2 of those, unspecified on overflow and has a technical undefined behavior (UB) with isdigit(str[i]) when str[i] < 0. Suggest instead to return 1 value, like SIZE_MAX, for all problems.

A name like atosize would add clarity over atost.


Consider instead strto(u)*() functions instead. To catch negative values, use a signed version. Oddly, strtou*("-1", ...) is "valid" non-overflow input.

static size_t atosize(const char *str) {
  char *endptr;
  errno = 0;  
  #if SIZE_MAX <= LONG_MAX
    long size = strtol(str,  &endptr, 0);
  #else
    intmax_t size = strtoimax(str,  &endptr, 0);
  #endif
  if (str == endptr) return SIZE_MAX;  // no conversion
  if (*endptr) return SIZE_MAX;  // Extra trail non numeric chracters
  if (errno) return SIZE_MAX;  // overflow
  if (size < 0 || size > SIZE_MAX) {
    errno = ERANGE;   // Set errno for consistent functionality on OF
    return SIZE_MAX;  // overflow
  } 
  return (size_t) size;
}

Additional work needed to pass INTMAX_MAX < size <= UINTMAX_MAX.

\$\endgroup\$
  • 1
    \$\begingroup\$ Why strtoimax() rather than strtoumax()? The latter will reject negative inputs with a range error for you. \$\endgroup\$ – Toby Speight Nov 22 '17 at 17:08
  • \$\begingroup\$ @TobySpeight strtou...() do not set errno with negative text inputs. "If the subject sequence begins with a minus sign, the value resulting from the conversion is negated (in the return type)." C11 §7.22.1.4 5 \$\endgroup\$ – chux Nov 22 '17 at 17:11
  • \$\begingroup\$ That's not my understanding: "If the converted value falls out of range of corresponding return type, a range error occurs (setting errno to ERANGE) and INTMAX_MAX, INTMAX_MIN, UINTMAX_MAX or ​0​ is returned, as appropriate." Negative numbers are not in the range of uintmax_t. \$\endgroup\$ – Toby Speight Nov 22 '17 at 17:14
  • 1
    \$\begingroup\$ Yes, I've just been doing a similar test (but with %ju also in the format string, to see the return value). I was disappointed to be proved wrong - it makes the function almost useless. \$\endgroup\$ – Toby Speight Nov 22 '17 at 17:21
  • 1
    \$\begingroup\$ @MarcoLucidi Detecting negative values is simplest with strtol(), strtoll(), strtoimax(), per the above comments. SIZE_MAX, some unsigned type, has a range somewhere about those 3 (unless its the same as uintmax_t. As C does not specify where, this answer steered code based on LONG_MAX for the sake of efficiency - why use strtoimax() when a narrow function would do? A more complete answer would select code based on LONG_MAX, LLONG_MAX, INTMAX_MAX. \$\endgroup\$ – chux Nov 26 '17 at 16:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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