I'm getting a stack overflow and I don't understand why

Question

I have been writing a program that converts a series of strings from the command line into one case based on the case of the first letter. The program itself works and runs fine, but when I use the address sanitizer, it keeps saying I am trying to read outside of my buffer.

Here is my code:

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

#define TRUE 1
#define FALSE 0

void print_list(char **data);

int main(int argc, char *argv[]) {
    if(argv[1] == NULL) {
        fprintf(stderr, "Error: No strings were entered\n");
        return EXIT_FAILURE;
    }

    int arg_num = 1;
    char *output[argc - 1];

    while (argv[arg_num] != NULL) {
        int count = 0;
        char *line;
        line = malloc(sizeof(char));
        int size = sizeof(char);
        int word_flag = FALSE;
        int upper_flag = FALSE;
        int lower_flag = FALSE;

        while (argv[arg_num][count] != '\0') {
            size = size + sizeof(char);
            line = realloc(line, size);

            if (argv[arg_num][count] >= 65 && argv[arg_num][count] <= 90) {
                if (word_flag == FALSE && upper_flag == FALSE) {
                    word_flag = TRUE;
                    upper_flag = TRUE;
                    line[count] = argv[arg_num][count];
                }
                else if (word_flag == TRUE && upper_flag == FALSE) {
                    line[count] = (argv[arg_num][count] + 32);
                }
                else {
                    line[count] = argv[arg_num][count];
                }
            }
            else if (argv[arg_num][count] >= 97 && argv[arg_num][count] <= 122) {
                if (word_flag == FALSE && lower_flag == FALSE) {
                    word_flag = TRUE;
                    lower_flag = TRUE;
                    line[count] = argv[arg_num][count];
                }
                else if (word_flag == TRUE && lower_flag == FALSE) {
                    line[count] = (argv[arg_num][count] - 32);
                }
                else {
                    line[count] = argv[arg_num][count];
                }
            }
            else if (argv[arg_num][count] == ' ' || argv[arg_num][count] == '\t') {
                word_flag = FALSE;
                upper_flag = FALSE;
                lower_flag = FALSE;
                line[count] = argv[arg_num][count];
            }
            else {
                line[count] = argv[arg_num][count];
            }

            count++;
        }
        
        line[count] = '\0';

        output[arg_num - 1] = line;

        arg_num++;
    }

    print_list(output);
    return EXIT_SUCCESS;
}

void print_list(char **data) {
    int count = 0;

    while (data[count] != NULL) {
        printf("%s\n", data[count]);
        free(data[count]);
        count++;
    }
}

And here is the output from the sanitizer:

==222362==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffc487cc7a8 at pc 0x000000401f8f bp 0x7ffc487cc740 sp 0x7ffc487cc738
READ of size 8 at 0x7ffc487cc7a8 thread T0
    #0 0x401f8e in print_list (/home/evanlewis/Documents/School/CS/306/Homework/Assignment 2/a.out+0x401f8e)
    #1 0x401e87 in main (/home/evanlewis/Documents/School/CS/306/Homework/Assignment 2/a.out+0x401e87)
    #2 0x7f0e7364a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f)
    #3 0x7f0e7364a5c8 in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x275c8)
    #4 0x401144 in _start (/home/evanlewis/Documents/School/CS/306/Homework/Assignment 2/a.out+0x401144)

Address 0x7ffc487cc7a8 is located in stack of thread T0
SUMMARY: AddressSanitizer: dynamic-stack-buffer-overflow (/home/evanlewis/Documents/School/CS/306/Homework/Assignment 2/a.out+0x401f8e) in print_list
Shadow bytes around the buggy address:
  0x1000090f18a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000090f18b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000090f18c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000090f18d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000090f18e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1000090f18f0: ca ca ca ca 00[cb]cb cb cb cb cb cb 00 00 00 00
  0x1000090f1900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000090f1910: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000090f1920: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000090f1930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000090f1940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==222362==ABORTING

According to the sanitizer, the problem is in print_list, the function that prints the strings from the array storing them post-conversion. It insists that I am trying to read outside of my buffer even though I would think the while loop would prevent that from happening. After all the strings have been printed, data[count] should equate to NULL, as there are only as many rows as strings. Is there something I am misunderstanding?

The stack trace would be more useful if it contained line numbers. You may want to read this: [How do I get line numbers in the debug output with clang's -fsanitize=address?](https://stackoverflow.com/q/24566416/12149471) — Andreas Wenzel, Mar 09 '23 at 03:33
What you are getting is not a [stack overflow](https://en.wikipedia.org/wiki/Stack_overflow), but rather a [buffer overflow](https://en.wikipedia.org/wiki/Buffer_overflow) on the stack. You are probably reading from an array out of bounds. — Andreas Wenzel, Mar 09 '23 at 03:39
Offering some critiques on your C style. Both of your while loops could be replaced with for loops. the first one would be ```for(int argvnum = 0; argnum < argc; argnum++)```. the second one would be ```for (int count = 0; argv[argnum][count] != '\0'; count++)```. this would make your code a bit easier to read. also you should do something like ```char line[strlen(argv[i])]``` instead of using ```malloc()``` and DEFINITELY not using ```realloc()``` for a small application like this — squidwardsface, Mar 09 '23 at 03:52
"dynamic-stack-buffer-overflow" Would mean something like: you have an overflow in a dynamic buffer (VLA) allocated on the stack. It's a poorly written diagnostic message. — Lundin, Mar 09 '23 at 07:37
@Evan I'd like to point out that basically, your code is overly complex. You can duplicate the *whole* strings you're getting from the command line, instead of processing a `malloc` for each character. Try to consider using `strdup`, so it will preserve your string terminator, and avoid a possible overflow cause. As you gain more experience with C you could consider other solutions as well. One more note: rely on `argc` to count the strings: it's there for that ... — LuC, Mar 09 '23 at 13:41
@ikegami Yeah, I understand that now, Like Lundin said its a poorly written diagnostic message. I'm just starting with C so I don't quite have the hang of memory management, and don't understand all the different types of memory errors. I mistook the dynamic buffer overflow for an actual memory overflow. — Evan Lewis, Mar 09 '23 at 22:02
@squidwardsface I appreciate the critiques. Why shouldn't realloc be used for small applications? — Evan Lewis, Mar 09 '23 at 22:03
@LuC Thanks for the feedback. I'll definitely remember that in the future. — Evan Lewis, Mar 09 '23 at 22:05

score 3 · Accepted Answer · answered Mar 09 '23 at 03:43

Your print_list method relies on the data array ending with a NULL pointer, but your code in main doesn't put a NULL pointer into that array, and doesn't have enough space in the array for a final null pointer.

For example, if you pass one argument to the program, argc will be 2 (one for the program name and one for the argument). This line:

char *output[argc - 1];

will create an array with space for 1 entry.

When your loop in print_list loops around, it will read past the end of the array as soon as count is > 0

while (data[count] != NULL) {

One way to fix this would be to ensure your array is large enough, and ensure that the last entry is always null.

char *output[argc];
output[argc - 1] = null;

I tried to do this before posting and was still having errors, but just tried again and it worked. I must have been messing something up when I tried earlier. Thanks for the help! — Evan Lewis, Mar 09 '23 at 21:53

I'm getting a stack overflow and I don't understand why

1 Answers1