aers / FFXIVClientStructs

Resources for reverse-engineering the FFXIV client's native classes.
MIT License
218 stars 152 forks source link

AtkTextNode.SetText string overloads set a pointer to stack in AtkTextNode #1040

Open aers opened 1 month ago

aers commented 1 month ago

Issue

It was recently discovered that AtkTextNode stores the pointer to the original text buffer used when calling SetText. The string overload generator handles converting C# strings (UTF-16) to strings the game expects (generally, null-terminated UTF-8). It relies on the fact that the game doesn't take ownership of the string buffer passed to the native function and temporarily allocates a buffer to store the converted UTF-8 string in, usually on the stack. The game then copies the string into its own objects and the string overload method can allow the buffer to be freed. Since AtkTextNode stores a pointer to the original text buffer, this results in the node storing a pointer to the stack.

While people have been using this function for years with no visible issues, it is incredibly unsafe for us to be storing a stack pointer in a native game object like this, so we can't provide string overloads for SetText anymore.

Plugin Authors

If you use SetText you will need to allocate the UTF-8 string buffer yourself and free it when your plugin is done using the TextNode.

The sample code below shows how to allocate and pass the buffer. It uses the same methods ClientStructs' string overloads uses to convert a string to UTF8.

byte* strBuffer;

unsafe void SetText(AtkTextNode* node, string text)
{
    if (strBuffer != null) // free buffer if you've already got one
        System.Runtime.InteropServices.NativeMemory.Free(strBuffer);
    int strLen = System.Text.Encoding.UTF8.GetByteCount(text); // get length of string as UTF-8 bytes
    strBuffer = System.Runtime.InteropServices.NativeMemory.Alloc(strLen + 1); // need one extra byte for the null terminator
    Span<byte> bufferSpan = new(strBuffer, strLen + 1); // wrap buffer in a span so you can use GetBytes
    System.Text.Encoding.UTF8.GetBytes(text, bufferSpan); // convert string to UTF-8 and store in your buffer
    bufferSpan[strLen] = 0; // add null terminator to the end of your string
    node->SetText(strBuffer);
}

You will also need to call Free when you are no longer using the TextNode (if you're destroying it, or your plugin is unloading, for example). You can also use other C# allocation functions (AllocHGlobal) or the game's native UI allocator (via IMemorySpace) if you want.

Excessive Allocations

You may be worried about allocating a new buffer every time you set the text. First - don't prematurely optimize. That said, the way to avoid this is to re-use the buffer.

The code examples below weren't directly tested, likely have typos, and probably need some more work to be complete.

byte* strBuffer;
int strBufferLen;

unsafe void SetText(AtkTextNode* node, string text)
{
    int strLen = System.Text.Encoding.UTF8.GetByteCount(text); // get length of string as UTF-8 bytes
    if (strBuffer == null || strLen + 1 > strBufferLen) // reallocate buffer if it doesn't already exist or is too small
    {
        System.Runtime.InteropServices.NativeMemory.Free(strBuffer);
        strBuffer = System.Runtime.InteropServices.NativeMemory.Alloc(strLen + 1); // need one extra byte for the null terminator
        strBufferLen = strLen + 1;
    }
    Span<byte> bufferSpan = new(strBuffer, strLen + 1); // wrap buffer in a span so you can use GetBytes
    System.Text.Encoding.UTF8.GetBytes(text, bufferSpan); // convert string to UTF-8 and store in your buffer
    bufferSpan[strLen] = 0; // add null terminator to the end of your string
    node->SetText(strBuffer);
}

You could wrap this logic in a class:

unsafe class AtkTextNodeBufferWrapper
{
    private byte* strBuffer;
    private int bufferLen;

    public byte* GetBuffer => strBuffer;

    public void SetBuffer(string text)
    {
        int strLen = System.Text.Encoding.UTF8.GetByteCount(text); // get length of string as UTF-8 bytes
        if (strBuffer == null || strLen + 1 > strBufferLen) // reallocate buffer if it doesn't already exist or is too small
        {
             System.Runtime.InteropServices.NativeMemory.Free(strBuffer);
            strBuffer = System.Runtime.InteropServices.NativeMemory.Alloc(strLen + 1); // need one extra byte for the null terminator
            strBufferLen = strLen + 1;
        }
        Span<byte> bufferSpan = new(strBuffer, strLen + 1); // wrap buffer in a span so you can use GetBytes
        System.Text.Encoding.UTF8.GetBytes(text, bufferSpan); // convert string to UTF-8 and store in your buffer
        bufferSpan[strLen] = 0; // add null terminator to the end of your string
    }

    public void FreeBuffer()
    {
        System.Runtime.InteropServices.NativeMemory.Free(strBuffer);
        bufferLen = 0;
    }
}

Also consider pre-allocating a buffer length you know is likely to store the longest text you're setting, especially if its a small amount of text.

Another option is to use the game's Utf8String class. This class has internal storage for a string of length 64 or smaller and won't reallocate unless your strings are larger.

Utf8String.FromString(str) allows you to allocate an unmanaged Utf8String* which you can then save. You can update the string text with SetString. The Utf8String's StringPtr can be safely passed to SetText.

aers commented 1 month ago

Leaving this open for a few days for questions.

Critical-Impact commented 1 month ago

Are there any special considerations we have to take with Addons? i.e. if we SetText on a AtkTextNode within an addon, do we need to free the memory when the addon closes?

aers commented 1 month ago

The TextNode dtor doesn't free the original buffer pointer so my assumption is the game is still treating you as the owner. If the game destroys the text node you can safely free the buffer. You'll just leak memory/be holding extra memory allocated if you don't.

Realistically if you mess up lifetime stuff you'll just be leaking some memory but won't cause a big issue; its not like the node is getting destroyed and recreated every frame.

MidoriKami commented 1 month ago

Using KamiToolKit as a platform for testing this out, is the following considered good practice:

    private readonly Utf8String* stringBuffer = Utf8String.CreateEmpty();

    public SeString Text {
        get => MemoryHelper.ReadSeStringNullTerminated((nint) InternalNode->GetText());
        set {
            stringBuffer->SetString(value.Encode());
            InternalNode->SetText(stringBuffer->StringPtr);
        }
    }

    protected override void Dispose(bool disposing) {
        if (disposing) {
            stringBuffer->Dtor(true);
            base.Dispose(disposing);
        }
    }
aers commented 1 month ago

I don't see why it wouldn't work

MidoriKami commented 1 month ago

Note for others that weren't present for the conversation in discord, when using a UTF8String as a buffer, ensure that the string you set to the UTF8String is null terminated.

Haselnussbomber commented 1 month ago

The issue came up, that Dalamuds SeString.Encode() doesn't add a null-terminator at the end of its byte[] and passing this directly to the Utf8String.SetString function (via the ReadOnlySpan<byte> overload) causes problems, because it expects a null terminator at the end.

Can the generator be changed, so that the ReadOnlySpan<byte> overload checks if the null-terminator is present, and adds it when necessary?

MidoriKami commented 1 month ago

Potentially cheesy fix would be using:

AtkTextNode->NodeText.SetString( value );
AtkTextNode->SetText( AtkTextNode->NodeText.StringPtr );