sprintf cuts output string by itself

Question

Here's my function:

char *HexCastName(UINT Address) {
    char Name[100] = "";
    UINT Offset;
    for (int i = 0; i < HardNames.size(); i++) {
        if (HardNames[i].Array == Hex.CurrentRegion.Array &&
            HardNames[i].Start <= Address &&
            HardNames[i].Start + HardNames[i].Size > Address) {
            Offset = Address - HardNames[i].Start;
            sprintf(Name, " : %s[%d]", HardNames[i].Name, Offset);
        }
    }
    return Name;
}

I do this:

char name[100];
sprintf(name, HexCastName(SELECTION_START));

The resulting string name is only 4 digits, though HexCastName() returns more. I tried to trace it, and it seems to pass the entire string to sprintf(), and somewhere inside it, it gets to _output_l(outfile,format,NULL,arglist) function. Inside it, the format variable at first contains my whole string, then after reading some variables the execution jumps from 973 to 978 in output.c, and my format is already truncated. I'm totally confused by that behavior... Why 4 letters? Maybe some failure with pointers and chars?

EDIT:

Here's the version that seems to work:

void HexCastName(char *buff, UINT size, UINT Address) {
    sprintf(buff, "");
    UINT Offset;
    for (int i = 0; i < HardNames.size(); i++) {
        if (HardNames[i].Array == Hex.CurrentRegion.Array &&
            HardNames[i].Start <= Address &&
            HardNames[i].Start + HardNames[i].Size > Address) {
            Offset = Address - HardNames[i].Start;
            _snprintf(buff, size, " : %s[%d]",HardNames[i].Name, Offset);
        }
    }
}

char *name = (char *)malloc(100);
HexCastName(name, 100, SELECTION_START);
sprintf(str, "%s: $%06X%s", area,
    Hex.AddressSelectedFirst + Hex.CurrentRegion.Offset, name);
free(name);

Answer 1

You are returning Name in the function HexCastName, which is a local array variable. After the function exits, it doesn't point to a valid place anymore.

Instead, use dynamic allocation:

char *Name = malloc(100);

Remember to free the memory when it's not used.

Answer 2

I the C programming language arrays have the same scoping rules as other variables. In your code char name[100] will be pushed to the stack but will be removed when the function returns.

char *HexCastName(UINT Address) {
    char Name[100] = ""; // Here `Name` gets pushed to the stack.
    UINT Offset;
    for (int i = 0; i < HardNames.size(); i++) {
        if (HardNames[i].Array == Hex.CurrentRegion.Array &&
            HardNames[i].Start <= Address &&
            HardNames[i].Start + HardNames[i].Size > Address) {
            Offset = Address - HardNames[i].Start;
            sprintf(Name, " : %s[%d]", HardNames[i].Name, Offset);
        }
    }
    return Name;
    // Here will `Name` be "deleted" as it exits scope, the pointer that is returned will point to garbage memory.
}

You have 3 options:

The first is to allocate the memory for Name on the heap in the function.

char *Name = malloc(100);

This is not recommended because you can easily forget to free the pointer that is returned:

void function()
{
    char *Name = HexCastName(0xDEADBEEF);
    free(Name); // If you forget to write this you will have a memory leak. Uugh.
}

The second approach:

You can declare Name static.

static char name[100];

If you don't know what static means that Name will always be in memory (like a global variable). This is quite nice because now we don't have to worry about freeing it's memory.

But this approach also has a big flaw. Multiple function calls will modify the same variable of Name. For example:

void function()
{
    char *Name1 = HexCastName(0xDEADBEEF);
    char *Name2 = HexCastName(0xDEAFCAEF);
    printf("Name1 = %s\n", Name1);
    printf("Name2 = %s\n", Name2);
    // Because we declared `Name` static will both pointers point to the same memory and point to the same string. Not very good.
}

This gets even worse because the function isn't declared to return a const char * but a modifiable char *.

The third solution:

The third solution is the one I recommend. It is to declare HexCastName like this:

// The user chooses were to write the result.
void HexCastName(char *NameOut, size_t BufSize, UINT Address) {
    char NameOut[0] = "";
    UINT Offset;
    for (int i = 0; i < HardNames.size(); i++) {
        if (HardNames[i].Array == Hex.CurrentRegion.Array &&
            HardNames[i].Start <= Address &&
            HardNames[i].Start + HardNames[i].Size > Address) {
            Offset = Address - HardNames[i].Start;
            snprintf(NameOut, " : %s[%d]", BufSize, HardNames[i].Name, Offset);
        }
    }
}

Please note that I used snprintf instead of the regular sprintf. This is because sprintf is vulnerable to "buffer overflows". If you want to read more about it you can check the accepted answer to this question: sprintf function's buffer overflow?

This version gets used like this:

void function(void)
{
    char Name[100];
    HexCastName(Name, sizeof(Name));
}

Answer 3

No that's not a problem of sprintf.

The reason is, you Name have its local scope and storage inside HexCastName, and passing the pointer outside the function is undefined behaviour.

To solve the problem, you could either use @YuHao's approach by dynamically allocating the storage, or have Name outside the functino HexCastName and pass its pointer to the function.

Answer 4

HexCastName returns a pointer to a local variable. The memory it's pointing to is no longer valid when HexCastName returns, and therefore you're passing garbage to sprintf.

You could make HexCastName allocate memory with malloc and return that, or you could have the caller responsible for passing in a buffer for HexCastName to write to.