Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've written a program that encrypts files (or stdin) with a user-specified key from 1 to 255. This "encryption" is very insecure, but still a bit better than ROT13 (at least for people trying to crack it using pencils and paper). How can I improve this program?

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

/*
 * A simple program for encrypting files (or stdin) using the XOR operation.
 * Such encryption is very insecure as there are only 254 valid key values,
 * but is still much more secure than ROT13, where there is only 1 valid key.
 *
 * Using a key value of 0 or 256 is equivalent to no encryption, and is
 * therefore not allowed. Other values are too large to fit into one byte
 * and therefore cannot be properly XORed with a byte.
 */

static void usage(const char *const);

int main(int argc, char *argv[])
{
    FILE *in = NULL;
    FILE *out = NULL;
    int k = 0;
    int c;

    while ((c = getopt(argc, argv, "i:o:k:h")) != -1) {
        switch (c) {
        case 'i':
            in = fopen(optarg, "r");
            break;
        case 'o':
            out = fopen(optarg, "w");
            break;
        case 'k':
            k = atoi(optarg);
            break;
        case 'h':
            usage(argv[0]);
            return EXIT_FAILURE;
        }
    }

    in = in ? in : stdin;
    out = out ? out : stdout;

    if (k <= 0 || k > 255) {
        fprintf(stderr, "%s: must specify a valid key via -k\n",
            argv[0]);
        fprintf(stderr, "%s: valid key values are from 1 to 255\n",
            argv[0]);
        return EXIT_FAILURE;
    }

    while ((c = fgetc(in)) != EOF) {
        fputc(c ^ k, out);
    }

    return EXIT_SUCCESS;
}

void usage(const char *const s)
{
    fprintf(stderr, "usage: %s -k key [-i input] [-o output]\n", s);
}

To compile:

$ clang -O2 -Weverything -Werror -o xor xor.c
share|improve this question

4 Answers 4

up vote 4 down vote accepted

It's a relatively small program and pretty well written, so there's not a lot on which to comment, but here are a few things that may help you improve your program.

Handle error conditions

As it stands, if the user specifies an input file that does not exist, fopen will return a NULL value. Rather than emitting some kind of error message, this causes the program to silently use stdin instead. This is not intuitive behavior and could lead the user into thinking that the program has hung rather than that it's just waiting for input.

Reorder functions to eliminate declaration

By putting the definition of usage above that of main where it is first used, the function declaration can be omitted. I think that makes the program easier to read and understand.

Declare local functions static

There is not likely to be any reason that usage needs anything more than static scope. That is, if the function is only going to be used from within this one file, declaring it static clearly conveys that (enforced) intent to the compiler and linker.

Eliminate return EXIT_SUCCESS at the end of main

Since C99, the compiler automatically generates the code corresponding to return EXIT_SUCCESS at the end of main so there is no need to explicitly write it.

share|improve this answer

Use binary mode

Currently, you are opening your files like this:

    in = fopen(optarg, "r");
    out = fopen(optarg, "w");

On a UNIX system that would be fine, but on a Windows system, that could result in CRLF pairs being converted into a single LF when reading, and single LF characters converted to CRLF pairs when writing. This is particularly important because once you do the xor operation, your output may contain random LF characters.

You should open your files in binary mode instead:

    in = fopen(optarg, "rb");
    out = fopen(optarg, "wb");
share|improve this answer

For your getopt() operation, you should also check for an invalid option. You can either check for '?', which is returned when an unknown option is found, or you can use a default case, or both. Seeing as your 'h' option is already handles program termination, you can just add a '?' case there, but you may still add a default case if the compiler complains about it.

share|improve this answer
    
Ah yes, that was stupid of me... Thanks for reminding me. I should also change the -h option to not return EXIT_FAILURE, instead returning EXIT_SUCCESS. – Anonymous Shadow 2 hours ago
    
@AnonymousShadow: Do you have the option meanings documented somewhere, or is it similar to an existing program? – Jamal 2 hours ago
    
They are pretty obvious when printed out in the usage() function. -i is to specify input file, if omitted, stdin is used. -o is to specify output file, if omitted, stdout is used. -k is for specifying the key, it is mandatory. – Anonymous Shadow 2 hours ago
    
@AnonymousShadow: What about -h? As you said earlier, it should've returned EXIT_SUCCESS, which means this shouldn't be used along with '?'. – Jamal 2 hours ago
    
... and -h is for reading the usage. I will edit my program to have the default case to show the usage and then return EXIT_FAILURE, to remove this mistake, then post a follow-up when I have more time. – Anonymous Shadow 2 hours ago

One thing I see is that your variable names are somewhat unclear. One letter variables are nearly always discouraged unless you're deliberately obfuscating your code OR are golfing your code for bytelength. And even if it's obfuscation, it's poor obfuscation since it's still easy to determine what the variables are from the source code.

If you HAVE to use one letter variable names for some reason, there is no reason why you can't make your basic source code easy to read and use a pre-build step to minify your code, especially with a program that's as trivial as this one. Instead of k use key, instead of c use charvalue.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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