4
\$\begingroup\$

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
\$\endgroup\$
12
\$\begingroup\$

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");
\$\endgroup\$
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Huh, i didn't know that about the implicit return code. TIL. \$\endgroup\$ – fluffy Dec 15 '15 at 21:59
5
\$\begingroup\$

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 already handles program termination, you can just add a '?' case there, but you may still add a default case if the compiler complains about not having one.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – user73265 Dec 12 '15 at 19:04
  • \$\begingroup\$ @AnonymousShadow: Do you have the option meanings documented somewhere, or is it similar to an existing program? \$\endgroup\$ – Jamal Dec 12 '15 at 19:06
  • \$\begingroup\$ 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. \$\endgroup\$ – user73265 Dec 12 '15 at 19:08
  • \$\begingroup\$ @AnonymousShadow: What about -h? As you said earlier, it should've returned EXIT_SUCCESS, which means this shouldn't be used along with '?'. \$\endgroup\$ – Jamal Dec 12 '15 at 19:10
  • \$\begingroup\$ ... 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. \$\endgroup\$ – user73265 Dec 12 '15 at 19:22
2
\$\begingroup\$

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. c is a convention and thus can stay like it is.

Sure, this is nitpicking and there are bigger mistakes in there that others have already pointed out, but even though the meaning of k can be inferred easily, it is still a good idea to stick to a good habit, even on smaller projects.

\$\endgroup\$
  • 1
    \$\begingroup\$ I disagree with this answer. There is only one variable k that should have a longer name; however, it is very easy to determine this variable's purpose. The other variables have proper names and, as for c, it is common practice in C to have the name c for a character that is being read from a stream. It is quite apparent that the OP is not trying to golf or obfuscate their code. \$\endgroup\$ – SirPython Dec 13 '15 at 0:34
  • 1
    \$\begingroup\$ @Sirpython I see. It has been a while since I last used C. I'm just biased against single letter variable names. Since c is common practise I will remove that once I can. \$\endgroup\$ – Nzall Dec 13 '15 at 10:53
  • 2
    \$\begingroup\$ Changing k to key would definitely be an improvement. In addition, I would change the first c to opt so as to avoid reusing the same variable for two very different purposes. \$\endgroup\$ – 200_success Dec 14 '15 at 5:03
1
\$\begingroup\$

Rather than reassign in and out to stdin/stdout if they're NULL, you could instead just set those as the default values:

FILE *in = stdin;
FILE *out = stdout;

Another thing you should do is handle the case that the user has multiple -i/-o switches, e.g.:

case 'i':
    if (in != stdin) {
        fclose(in);
    }
    in = fopen(optarg, "rb");
    break;
case 'o':
    if (out != stdout) {
        fclose(out);
    }
    out = fopen(optarg, "wb");
    break;

However, in that case, multiple -o switches will create multiple files, so it might be better to just fail as an error. In fact, it might even be better to have the switches store the filenames instead:

char *in_file = NULL;
char *out_file = NULL;

while ((c = getopt(...))) {
    switch (c) {
    case 'i':
        free(in_file); // no-op if already NULL
        in_file = strdup(optarg);
        break;
    case 'o':
        free(out_file);
        out_file = strdup(optarg);
        break;
    // ...
    }
}

/* Some people will take issue with this for having a C99-style variable
   declaration. However, C99 has been around for almost 17 years now. It
   is no longer "too new," IMO. :)
*/
FILE *in = in_file ? fopen(in, "rb") : stdin;
if (!in) {
    perror(in_file);
    return 1;
}
free(in_file);

FILE *out = out_file ? fopen(out, "wb") : stdout;
if (!out) {
    perror(out_file);
    return 1;
}
free(out_file);

Also, it's a soft UNIX convention to just use a positional argument to specify the input file (and often output file), and to use - to indicate stdin/stdout. getopt mutates argc and argv such that they contain just the positional arguments after the while loop is complete.

\$\endgroup\$

Your Answer

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