0

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);
feos
  • 1,099
  • 1
  • 9
  • 19

4 Answers4

3

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.

Yu Hao
  • 119,891
  • 44
  • 235
  • 294
  • @feos When you don't need it any more. – Yu Hao Sep 21 '14 at 08:33
  • Well, I don't need it after return I guess, but it exits right after returning. It must exist for some time to get to another function's space, but then there's no `Name` variable anymore, what to free? – feos Sep 21 '14 at 08:37
  • @feos Store the return value into a variable like `char *foo = HexCastName(SELECTION_START);` then later `free(foo);`. – Yu Hao Sep 21 '14 at 08:39
  • Please verify the version I posted in the question. – feos Sep 21 '14 at 08:49
  • I would not recommend this. I explained why in my answer. – wefwefa3 Sep 21 '14 at 09:21
3

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));
}
Community
  • 1
  • 1
wefwefa3
  • 3,872
  • 2
  • 29
  • 51
  • +1. The third solution is another good option, but it also has its cons: after its use, `Name` cannot be freed like the first solution. – Yu Hao Sep 21 '14 at 09:33
  • Looks like I just need to malloc in the main area, not in `HexCastName()`, so malloc and free will be really close to each other and very obvious. – feos Sep 21 '14 at 09:37
  • @YuHao You can also call the function like this: `char *Name = malloc(100); HexCastName(Name, 100); free(Name);`. @feos Yes, that is the advantage of this version. – wefwefa3 Sep 21 '14 at 09:41
  • Edited the question again, to apply everything. Verify please. – feos Sep 21 '14 at 09:57
  • @feos I cannot test the code as I don't have the decleration of `HardNames` and `Hex`. But I think that the code looks alright. `snprintf` might not work for you if you are using Microsofts C compiler (because `snprintf` is C99) but I haven't tried it. Try it yourself and if you find something that doesn't work with my code, write what here and I will try to help you and perhaps fix some mistakes I might have made (if neccesary). – wefwefa3 Sep 21 '14 at 11:25
1

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.

starrify
  • 14,307
  • 5
  • 33
  • 50
1

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.

jamesdlin
  • 81,374
  • 13
  • 159
  • 204
  • The second solution sounds intriguing. Can you provide some examples? I tried that, and didn't figure out the correct way. – feos Sep 21 '14 at 09:02