C - "Run-Time Check Failure #2 - Stack around the variable 'cstringValue' was corrupt-CodePudding

This code is to check whether the length of the user input is within the range of the lower and the upper limit. Or if upper limit and the lower limt are equal, testing whether the string length equal to variable 'equal'. For now, I am trying to return the string STEVEN to the main function but it keep on pops up "Run-Time Check Failure #2 - Stack around the variable 'cstringValue' was corrupted." The code works fine when the number of length does not equal to variable 'equal'.

I've tried the following:

The function code:

#define _CRT_SECURE_NO_WARNINGS
#include <stdio.h>
#include "core.h"
#include <string.h>
void inputCString(char *charPointer[], int lower, int upper)
    {
        int count, equal;
        
        
        if (upper != lower)
        {
            
            goto notEqual;
            
        }
        else if (upper == lower)
        {
            goto upperEqualLower;
        }
        notEqual:
            do
            {
                scanf("%s%n", *charPointer, &count);
                count--;
                if (count > upper || count < lower)
                {
                    printf("ERROR: String length must be between %d and %d chars: ", lower, upper);
                }
                else
                {
                    
                    return *charPointer;
                }

            } while (count > upper || count < lower);
        
    upperEqualLower:
        do
        {
            equal = upper;
            
            scanf("%s%n", &*charPointer, &count);
            count--;
            
            if (count != equal)
            {
                printf("ERROR: String length must be exactly %d chars: ", upper);
            }
            else if (count == equal)
            { 
                
                return *charPointer;
            } 
        } while (count != equal);

The main:


    char cstringValue[7] = { '\0' };

    
    printf("TEST #6: - Instructions:\n"
        "1) Enter the word 'horse'   [ENTER]\n"  // too short
        "2) Enter the word 'chicken' [ENTER]\n"  // too long
        "3) Enter the word 'STEVEN'  [ENTER]\n"  // just right
        ":>");

    
    inputCString(cstringValue, 6, 6);

    printf("////////////////////////////////////////\n");
    printf("TEST #6 RESULT: ");
    printf("%s (expected result: STEVEN)\n", cstringValue);
    printf("////////////////////////////////////////\n\n");

Thanks in advance.

CodePudding user response：

The type of your charPointer parameter is char** – while you dereference it correctly for first scanf (inequality case), you don't do so for second one (equality case: &*somePointer is equivalent to just somePointer). This is undefined behaviour, as the pointer type does not meet the format specifier.

Additionally, you need to create a pointer to pointer of cstringValue to be passed to the function – and simply doing &cstringValue will provide a pointer to array, which again is of bad type. So you need: char* ptr = cstringValue; inputCString(&ptr, ...);. The compiler should have warned you about mismatching pointer types (if not, increase warning level!), and you absolutely should listen to! (You might potentially get away with in the equality case as you provide mismatching pointers twice and the errors might compensate each other – still that remains undefined behaviour, the code working is pure luck).

Much simpler, though, is just turning the parameter in an ordinary char* pointer, then you can pass it to scanf directly without dereferencing and passing the input array to the function as is now gets correct again as well.

Depending on your input, you risk undefined behaviour by writing beyond array bounds as well – consider input like "dragonfly", which does not fit into the array! scanf still will try to write to, as you do not limit the input length – the length check (count) only occurs after the data has already been written to the array!

Providing a longer buffer reduces the risk of, but does not eliminate it. To be safe, you need to limit scanf to scan only up to some maximum. Unfortunately the maximum cannot be provided by passing an additional argument to scanf as with printf, so you need to adjust your format string accordingly, either using a constant string:

#define MAX_INPUT 7
#define ARRAY_SIZE (MAX_INPUT   1) // need space for a null-terminator!
#S(TEXT) S_(TEXT)                  // stringifying macro
#S_(TEXT) #TEXT                    // need indirection for

scanf("%" S(MAX_INPUT) "s", ...);

// quite a lot of work to be safe, I know...

or dynamically:

char format[8];
sprintf(format, "%%%ds%%n", length - 1);
// - 1: need to leave space for the null terminator
// (assuming you provide length as an additional parameter)

scanf(format, ...);

There are quite a number of further issues in your code:

You try to return *charPointer;, but return type of the function is void.

if(condition) {} else if(complementary condition) { } – if the initial condition has not been met then the complementary one must – so if getting into the else branch the second if is always met and you should just drop it: if(condition) {} else {}.

There are valid use cases for goto (like exiting from nested loops), but this one here isn't. You could just place the respective code blocks into the body of the if and else branches. Apart from, you do not have to differentiate between lower and upper differing at all, as if both values are equal one of count < lower or count > upper will apply if count is not equal to any of them (they are equal, remember?). Well, the error message differs, but you could select it before the loop: `char const* errorMsg = upper == lower ? "exactly" : "between;".

More interesting would be an additional check for lower <= upper, combined with appropriate error handling if not, as this would definitely lead to an endless loop.

count-- – why are you reducing it? "%n" provides the number of characters read, not the number of characters written to the array (the latter includes a null-terminator, the former not). So if 3 characters have been read for "abc", you end up with 2 and I doubt that is what you really want...

Labels do not produce any scope – they are just a marker where to continue execution in case of a goto but are otherwise ignored. That means that if you complete the code after notEqual it will just continue with the code after upperEqualLower. It appears unclear to me if you are aware of that, so noticing – in your specific case the fall-through is prevented by returning from the function before reaching the next label, so not an issue here. You won't run into this at all if you follow the recommendation to goto above, though (moving into the if/else blocks).

Your loops don't need to check any condition: You check exactly the same condition already within and break the loop by returning, so if reaching the end of the loop body the condition is true and you continue anyway. You could instead simply have a for(;;) loop.

Fixing all these issues your code like look like this:

void inputCString(char charPointer[], size_t length, size_t lower, size_t upper)
// size_t: correct type for specifying array or object sizes; aditionally
// it's unsigned, as negative values for lower and upper are meaningless anyway
{
    if(upper < lower)
    {
        // error handling!
    }

    char const* errorMsg = lower == upper ? "exactly" : "in between"; // examples

    char format[8];
    sprintf(format, "%%%ds%%n", length - 1);
    int count;
    for(;;)
    {
        scanf(format, charPointer, &count);
        // personal preference of mine: place the return into the if block
        // -> invert condition
        if (lower <= count && count <= upper)
        {
            return;
        }

        // surplus argument in case of equality is no problem:
        printf(errorMsg, lower, upper);
    }
}