Rot13 encryption with extra functionality using unix flags

Question

I just wanted to check out options/unix flags and read from stdio through pipe. Is it any good? Where should I improve or use something else? Feedback would be much appreciated.

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

void printHelp(void) {
    printf("Usage: rot [OPTION]... INPUT\n"
            "A simple letter substitution cipher that shifts each letter in the input text by a certain number of positions.\n"
            "By default, the text is shifted by 13 positions (ROT13)\n\n"
            "Options:\n"
            "  -c X\t\tUse a custom letter shift of 'X' positions instead of the default 13. 'X' must be a positive integer between 1 and 25.\n"
            "  -d\t\tDecrypt the input text by shifting letters backwards (reverse the cipher).\n"
            "  -h\t\tDisplay this help message and exit.\n\n"
            "Examples:\n"
            "  ./rot \"Hello World\"         # Encrypt the text with the default 13-shift (ROT13)\n"
            "  ./rot -c 5 \"Hello World\"    # Encrypt the text with a custom 5-shift\n"
            "  ./rot -d \"Uryyb Jbeyq\"      # Decrypt the text by shifting letters backwards (ROT13 reverse)\n"
            "  ./rot -c 3 -d \"Khoor Zruog\" # Decrypt the text with a custom 3-shift\n");
}
char shiftChar(char c, int rotateCount, int decrypt) {
    int offset = decrypt ? -rotateCount : rotateCount;
    if (c >= 'A' && c <= 'Z') {
        return ((c - 'A' + offset + 26) % 26) + 'A';
    } else if (c >= 'a' && c<= 'z') {
        return ((c - 'a' + offset + 26) % 26) + 'a';
    } else {
        return c;
    }
}

int main(int argc, char *argv[])
{
    int rotateCount = 13;
    int decrypt = 0;

    int opt;
    while ((opt = getopt(argc, argv, "dc:h")) != -1)
        switch (opt) {
            case 'h':
                printHelp();
                return EXIT_SUCCESS;
            case 'd':
                decrypt = 1;
                break;
            case 'c':
                rotateCount = atoi(optarg);
                if (rotateCount < 1 || rotateCount > 25) {
                    fprintf(stderr, "Invalid shift value. Please provide an integer between 1 and 25.\n");
                    return EXIT_FAILURE;
                }
                break;
            default:
                printHelp();
                return EXIT_FAILURE;
        }
    char *text;
    size_t length;
    if (optind >= argc) {
        size_t buffer_size = 256;  
        char *buf = malloc(buffer_size);
        if (buf == NULL) {
            fprintf(stderr, "malloc failed");
            return EXIT_FAILURE;
        }
        size_t read_size = 0;
        while (fgets(buf + read_size, buffer_size - read_size, stdin)) {
            read_size += strlen(buf + read_size);
            if (read_size >= buffer_size - 1) {
                buffer_size += 256;
                buf = realloc(buf, buffer_size);
                if (buf == NULL) {
                    fprintf(stderr, "realloc failed");
                    return EXIT_FAILURE;
                }
            }
        }
        length = read_size;
        text = buf;
    } else {
        text = argv[optind];
        length = strlen(text);
    }

    char rot[length];
    for (int i = 0; i < length; i++) {
            rot[i] = shiftChar(text[i], rotateCount, decrypt);
    }
    for (int i = 0; i < length; i++)
        printf("%c", rot[i]);
    printf("\n");
    return EXIT_SUCCESS;
}

Answer 1

Zoom out and consider...

You've shown good appreciation of C's ability to concatenate literal strings (the multi-line "help text").

Your method is perfectly valid.
An alternative could be to store the address of that text into a pointer variable, eliminate the function that encloses it, then, change the two invocations of that ex-function to
printf( myHelpVerbiage );,
or puts( myHelpVerbiage ); to neutralise any embedded single % characters that may be in the verbiage. This is one of C's nefarious 'traps for the careless' that is easily fallen-into (even by pro's).

Going BIG, a naïve or malicious user running your executable with command line redirection to a file or a pipe would, if using for instance "-x" on the command line, feed that text to the pipe or the file. "Help text" should go to stderr to be read by a human, or whatever logging is done of error/misfire invocation. Read up on fputs(), too, and consider its use here.

Avoid too quickly presuming you've got all the usage scenarios covered...

A comment from @DavidConrad (thank you David!) highlights a subtle distinction the code should accommodate. There's "help text" and there's "usage text".

The former should go to stdout: fputs( myVerbiage, stdout ); User forgot usage syntax.
The latter should go to stderr: fputs( myVerbiage, stderr ); A CLI error detected.

There are myriad ways of dealing with problematic real world inputs and data. Endless pursuit; plug-up one crack only to notice the next one, ad infinitum. (Great for job security if you play your cards right!)

And, YES! Protecting the executable from "user abuse" is one of the most difficult aspects of writing code! Foolproof?? NEVER underestimate the ingenuity of a fool! If there's a way to make your program go off the rails, one day a 'fool' will find how to trigger that trainwreck. All you can do is do YOUR best to make THEM work for it.

---

Missing

Usage option examples do not include an example when program is used as a Unix filter, only "command line parameter" examples shown. Easy to fix.

---

atoi() vs strtol()

In the gentler, more trusting days of C's inception, atoi() was sufficient. There were no (or fewer) bad actors around. In 2025, code needs to be more robust. Read up on strtol() and appreciate how it can be used to not only work with different bases, but its 3rd parameter can be used to detect the difference between "123" and "123FrogsAndToads". This WILL be important as you write defensive code for more intricate problems.

No fault to you, but atoi() is a sure sign of a beginner/amateur coder. It remains in C's standard library for compilers to be backward compatible for code written in a time when smoking in theatres was allowed.

Renovating legacy code (replacing atoi() with strtol()) can be like playing "Whack a Mole". Yes, one thing has been 'improved', but a previously 'dormant' bug may be awoken by the altered machine code. Related concept: trying to replace one Jenga piece late in the game. Sometimes it's wiser to leave well enough alone. Otherwise, a 10-minute fix may become a week-long misery.

---

Flow

Examine the if/else cascade in shiftChar(). Think about whether-or-not else is necessary in the else if and else lines. Just to say, return; is pretty absolute about what happens next. Code simplification while maintaining correct operation is highly desirable.

Try to form a practice of walking away for a few minutes, then returning to see if you can improve the 'flow' of the logic so it reads like a poem - easy to swallow without having to chew too much...

---

malloc() & realloc()

The executable likely has at least 1Mb of scratchpad "stack" available.

Consider reading into a fixed-size "stack string buffer", measuring what's been read, then starting or growing the dynamic buffer being assembled as required, with the tiny expense of having to copy (append) a bunch of bytes from stack buffer to dynamic memory buffer.

Left as an exercise is to really examine the doco for realloc() and eliminate the anticipatory malloc() you've written.

And, most important, always type free() whenever you type any of the names of the alloc-brothers. Never forget that what's borrowed must be returned. Some programs may be expected to run continuously, borrowing and releasing limited system resources so as not to deplete them to zero.

Sometimes you will be in the zone, conceiving and typing code, keystrokes sounding like rain on a rooftop. If you habitually type free(); on the line after malloc() or a sibling, it's unlikely the compiler will let you forget to move that operation to a suitable place. Your 'zone' is not disturbed, and you can continue to pour out all that's racing through your mind until you're exhausted. When the pace has slowed and tentative compiling starts, free() won't have been accidentally forgotten. Many IDE editors insert {} when { alone is typed. Think of malloc() and free() similarly. "You can't have one without the other".

---

Kitchen sinks are for kitchens, not code

I've suggested you eliminate the one "worker function" in this code. In its place, I suggest you see the body of main() as three distinct phases: set-up, do-stuff, and report (plus the missing tear-down of the absent free()). Each of these 'phases' really wants to be isolated into its own much simpler function. This is "refactoring" to follow The KISS Principle. Computers don't care, but human readers 'grok' things better as small chunks, one at a time. Further, it's easier to write/modify capsules of code and diagnose a bug when its range is restricted.

Don't throw "even-the-kitchen-sink" into an overly-long function. Break things down as much as you see fit. (One can overdo this; experience will establish your own comfort zone of factoring code into fragments.)

---

Reversable?

If a sender uses 5 to encipher a message, wouldn't it be easier for the receiver to use 5 (instead of 21) to decipher the ciphertext?

Consider the very minor tweak needed to provide this convenience.
Some are not so great at summing, or visualising inverted relationships.

A first-time user trying to decipher may wonder whether they can use -d OR skip it and enter -c -5... I hope you see what I mean...

Maybe consider simplify the UI to -e 5 and -d 5 only. One means "encipher", the other "decipher", and the required next parameter is always a positive magnitude...

Keep or get rid of "defaults to 13". Ask your friends what they'd prefer if they were to be users of this program. I'd prefer no defaults, myself. It's no big ask of any user to be explicit to use your program. (Becomes a non-optional parameter to the program! If default of 0 has not been changed, then terminate telling user -e # or -d # is a required parameter.)

---

Conclusion

All in all, very good code for a neophyte. Some hiccups, but nothing too major. A minor niggle over the final two for()'s and braces (one has, one has not). A minor inconsistency that could be improved upon.

Otherwise... Welcome to the club! Hope you come to love C as much as so many have before you.

---

Ya know...

That final printing for() loop could be replaced harnessing the power of printf().

    printf( "%.*s\n", length, rot );

If you read, and read again, and again, the doco for printf() you'll discover it has quite astounding capabilities! The format string used here informs printf() to only try to output length characters of the char [] at address rot. No '\0' to make a C string is required in this case.

This is but one "power user" technique you'll be master of by keeping a copy of the doco for the C Standard Library at your bedside. Read a bit before turning off the light every night. Most of the basic functionalities you'll ever need are already available. With its libraries, C is not as primitive as its reputation has people thinking.

And, now that we've save a few lines of code, we could consider how to handle being asked to encipher nothing (an empty file or "" as a parameter string). Should we report the number of characters processed to stderr, or may terminate with a FAILED message and error code??
Decisions, decisions!

To consider: If the user starts your program for it to encode from the standard input, your program will sit listening for any/all text provided until the user types whatever EOF key combo is appropriate for their OS. You may get a call at 2AM from a user complaining "your program just goes off into an endless loop." Perhaps, if stdin is the source of the data, your program could print a prompt to stderr that contains the key combo to signal EOF. Maybe even, "You may commence entry now. >>" Maybe add this extra user advisory of EOF key combo to the help text that's displayed for "-h"...

---

Overtime

With a few minor fix-ups or adjustments, there are no catastrophic problems with your code. Kudos.

However, thinking in a KISS frame of mind, there's a lot of complexity to this involving spooling-up its source data, 'batch' processing of the data, then spooling-out the results. This may have been the crux of your taking this approach.

Consider stripping out a lot of the guts to leave behind only the enciphering that deals with input one character at a time, writing each cipher-char to its output. Easy-peasy. This is how many Unix 'filters' are written; imposing no preset (or even dynamic) limits on the size of source data they can process. stdin is buffered, as is (normally) stdout. The application does not necessarily need to supply its own buffers to function.

KISSing is always good!

Answer 2

Allow wider offset range

Modular math with negative values incurs issues.

OP has safeguarded with if (rotateCount < 1 || rotateCount > 25) { yet consider a more liberal approach:

//if (rotateCount < 1 || rotateCount > 25) {

rotateCount %= 26;
if (rotateCount < 0) rotateCount += 26;
// Then no need for a `if (rotateCount < 1 || rotateCount > 25)` test.

Also then later repetitive code can simplify.

//return ((c - 'A' + offset + 26) % 26) + 'A';
return ((c - 'A' + offset) % 26) + 'A';

Answer 3

Reinventing the wheel

In your shiftChar function you can use the isupper and islower functions from <ctype.h>. Be careful to read the documentation for those functions, as you may need to cast from char to unsigned char.

char shiftChar(char c, int rotateCount, int decrypt) {
    int offset = decrypt ? -rotateCount : rotateCount;
    if (isupper(c)) {
        return ((c - 'A' + offset + 26) % 26) + 'A';
    } else if (islower(c)) {
        return ((c - 'a' + offset + 26) % 26) + 'a';
    } else {
        return c;
    }
}

realloc and dangling pointers

Also, generally when using realloc you want to assign the result to another variable, rather than back to the same variable. If realloc fails, you'll still be able to access the original block of memory and free it.

If you don't do this, that memory is left dangling and can't be explicitly freed.

        while (fgets(buf + read_size, buffer_size - read_size, stdin)) {
            read_size += strlen(buf + read_size);
            if (read_size >= buffer_size - 1) {
                buffer_size += 256;
                char *tmp = realloc(buf, buffer_size);
                if (tmp == NULL) {
                    fprintf(stderr, "realloc failed");
                    free(buf);
                    return EXIT_FAILURE;
                }

                buf = tmp;
            }
        }

Buffer growth strategies

It's worth noting that you grow your buffer by a constant factor of 256. This is much better than growing it by a much smaller number, but you might consider growing by multiplying by 2¹. Each time you reallocate you may incur the penalty of having to copy the contents of the array. Growing by a constant factor means that will occur a linear number of times as your buffer needs to grow.

If you multiply by two each time, the reallocations happen in a logarithmic fashion.

You probably also don't want to update buffer_size until you know that the reallocation succeeded.

        while (fgets(buf + read_size, buffer_size - read_size, stdin)) {
            read_size += strlen(buf + read_size);
            if (read_size >= buffer_size - 1) {
                char *tmp = realloc(buf, buffer_size * 2);
                if (tmp == NULL) {
                    fprintf(stderr, "realloc failed");
                    free(buf);
                    return EXIT_FAILURE;
                }

                buf = tmp;
                buffer_size *= 2;
            }
        }

Style

Your switch statement in main I can't help but feel that your two cases that exit the program early should be grouped together.

    while ((opt = getopt(argc, argv, "dc:h")) != -1)
        switch (opt) {
            case 'd':
                decrypt = 1;
                break;
            case 'c':
                rotateCount = atoi(optarg);
                if (rotateCount < 1 || rotateCount > 25) {
                    fprintf(stderr, "Invalid shift value. Please provide an integer between 1 and 25.\n");
                    return EXIT_FAILURE;
                }
                break;
            case 'h':
                printHelp();
                return EXIT_SUCCESS;
            default:
                printHelp();
                return EXIT_FAILURE;
        }

Undefined behavior

Also, within this switch statement you call atoi. You may wish to consider using strtol instead as a string that cannot be represented as an int will lead to undefined behavior when passed to atoi.

---

¹ There is some debate on the ideal growth factor for such a dynamic array/buffer. This has been covered on Stack Overflow.

Answer 4

Character encodings

Your shiftChar() function assumes that the unaccented upper- and lowercase Latin letters will be encoded in contiguous ranges of character codes, in alphabetical order. This is true of most computer systems, but C does not require it, and there are computer systems where it is not true. This function would need to take a different approach to work on such systems.

It is possible to write shiftChar() such that it does not have such a limitation, but in practice, I would never expect a beginning C student to come up with such a thing. I raise this mainly because that beginning C student ought to be taught early on that there is a consideration of this kind at all, even if we sometimes choose to ignore it.

File slurping

This program reads the whole input into memory, then operates on it, then writes the whole result out. This mode is necessary for some kinds of operations, but not for the one this program is performing. It is desirable to avoid that behavior where it is unnecessary. Where avoiding it is possible, doing so often makes the code simpler, always caps the amount of memory needed, and always makes the program more responsive.

Consider: in this program, every input character results in exactly one output character, and that character is determined by the command-line parameters and input character alone, without any contribution from any of the other input characters. Why, then, go to all the trouble to manage a dynamically-adjusted buffer?

A minimal version of this program could use getchar() to read input and a single int as all the buffer it needs. It would encipher each character immediately after reading it, and it would write the result before continuing to the next input character. If you want to do block reads, perhaps in an attempt to improve throughput, then a fixed-size, statically or automatically allocated buffer would accommodate that just fine. You would read bytes into the buffer, encipher them, write the results, then repeat with the same buffer.

And since you brought up use in a pipeline, consider that a pipeline can be fed by a program or device that produces output indefinitely. If you tried to use your program in a pipe to encipher the output of such a program then it would never produce any output, because it would never reach the end of the input. It would just keep allocating more and more memory to buffer more data, until the system refused to provide any more. At least it would then emit an informative diagnostic before terminating (kudos for that).

I/O functions

I mentioned already that you could implement the input with getchar(). Block reads are a fine idea too, but fgets is a suboptimal choice for that. One uses fgets() when one wants to break the input into lines, but doing so serves no purpose for your program. The point of block reads would be to improve performance, so why saddle that case with the extra work of watching for line terminators and mucking with string terminators? For block reads for this program, fread() would be a better choice.

And if you have a performance focus justifying block reads, then printf("%c", rot[i]) is a poor choice for output. putchar(rot[i]) would be better, and a worthy counterpart to reading input with getchar(), but if you want to perform block reads for input then the natural counterpart to fread() would be fwrite(), with which you would perform corresponding block writes.

Pointers

It occurs to me to wonder whether you chose dynamic allocation after having decided on fgets() and observed that its first argument is a pointer. If so, then get your C career off to a good start by pounding it into your brain that pointers do not imply dynamic allocation. Almost every C program uses pointers, one way or another, whether the programmer realizes it or not, but plenty of them do not use dynamic memory allocation. When I someday write my C textbook, I will introduce pointers in about chapter 3, and dynamic memory management in about chapter 7.

The most common way to get object pointers is probably through automatic conversion of array values, and the second is through use of the unary & (address of) operator. You should learn about these C features and understand them well -- both what they do and when and why to use them -- before you start working with dynamic memory allocation.

Variable-length arrays

This ...

    char rot[length];

... declares rot as a variable-length array (VLA). This feature does not appear until chapter 8. In chapter 2, I tell you the fib that array dimensions must be declared using constant expressions. That's because

• it's actually true for some C implementations. Support for direct declaration of VLAs is an optional language feature, and not all implementations provide it. In C11 and C17, all support for VLAs is optional. Only (obsolete) C99 makes full VLA support mandatory.

• Direct-declared VLAs are risky unless their size is carefully limited. The typical VLA implementation allocates them on the stack, which usually has fairly limited space. It is all too easy to try to allocate more than the system can accommodate, and you don't necessarily detect this during testing. And if that happens then there is no mechanism for handling or recovery. In your particular case, I could probably make your program crash at the VLA allocation by feeding it a large enough input.

  If you can strictly bound the amount of space needed then it can be safe, but if you can do that then you can instead use an ordinary, fixed-size array.

Unneeded allocation

... and you don't even need that VLA in the first place! You don't need the input data other than to compute the output, and any given input character contributes only to one output character. Therefore, you could just as well do the conversion in place, overwriting the input with the rotated output -- or even not storing it at all if you did something like putchar(shiftChar(text[i], rotateCount, decrypt)).

Don't be shy about allocating memory when that serves a useful purpose, but don't be needlessly wasteful, either. Modern computers have a lot of memory, but it is not unlimited. Large memory usage by one program does impact the overall system.

Dynamic allocation

When you do perform dynamic memory allocation then you should train yourself to good allocation discipline:

• every successful allocation or reallocation operation should, eventually, be complemented by exactly one subsequent, corresponding reallocation or deallocation operation. You can relax this a bit for memory blocks that you intentionally plan to hold all the way to program termination, or for abnormal program termination cases, but freeing every byte you dynamically allocate will make it easier to analyze your program with tools such as Valgrind, to help you detect bona fide memory leaks.

• reallocation results typically should not be stored directly in the variable holding the original pointer. In the event that the reallocation fails, that (typically) leaks the memory that was to be reallocated. This can be forgiven, however, in the event that reallocation failure is handled by promptly terminating the program.