FFXIVClientStructs icon indicating copy to clipboard operation
FFXIVClientStructs copied to clipboard

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

Open aers opened this issue 1 year ago • 8 comments

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 avatar Jul 14 '24 10:07 aers

Leaving this open for a few days for questions.

aers avatar Jul 14 '24 10:07 aers

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?

Critical-Impact avatar Jul 14 '24 10:07 Critical-Impact

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.

aers avatar Jul 14 '24 10:07 aers

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);
        }
    }

MidoriKami avatar Jul 14 '24 17:07 MidoriKami

I don't see why it wouldn't work

aers avatar Jul 15 '24 00:07 aers

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.

MidoriKami avatar Jul 15 '24 06:07 MidoriKami

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?

Haselnussbomber avatar Jul 16 '24 15:07 Haselnussbomber

Potentially cheesy fix would be using:

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

MidoriKami avatar Jul 17 '24 23:07 MidoriKami

Closing this as it is now resolved.

wolfcomp avatar Nov 06 '24 13:11 wolfcomp