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);
1
  • You are returning the address of an automatic-storage var - UB. Commented Sep 21, 2014 at 8:26

4 Answers 4

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));
}
Sign up to request clarification or add additional context in comments.

5 Comments

+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.
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.
@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.
Edited the question again, to apply everything. Verify please.
@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).
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.

5 Comments

@feos When you don't need it any more.
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 Store the return value into a variable like char *foo = HexCastName(SELECTION_START); then later free(foo);.
Please verify the version I posted in the question.
I would not recommend this. I explained why in my answer.
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.

Comments

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.

1 Comment

The second solution sounds intriguing. Can you provide some examples? I tried that, and didn't figure out the correct way.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.