Win32: freeing memory allocated in another function

I've run into a kind of strange situation: I have a function that takes a pointer-to-a-pointer (getting a string from a Win32 API call) and it works as expected, but with a memory leak I'm not sure how to fix. The string is allocated on the heap, but when I try to free it, Visual Studio shows a really cryptic "heap corrupt" message box. I get the gist of what it's saying - it doesn't like my call to free() because {reasons}, but I'm not sure I understand what those {reasons} are (and it won't let me copy the message box text with Control-C, so sorry I don't have the gibberish lol). But here's the code:

Code:

---

bool myFunction(char** output) {
    // NOTE: Before I did this part, I did all the usual stuff
    // required (OpenClipboard, GetClipboardData etc.)
    // Lock the handle to get the actual text pointer
    char* pszText = (char *)(GlobalLock(hData));
    if (pszText == NULL) return false;

    // Copy the data into our output variable
    size_t length = strlen(pszText) + 1;
    char* data = (char*)malloc(length);
    if (data == NULL) {
        CloseClipboard();
        return false;
    }
    data[length] = NULL;  // This was due to another weird VS error; apparently the Win32 calls don't null-terminate clipboard data automatically, so I'm having to do it manually here.  Could be the culprit, but that's unlikely because commenting this out didn't fix the problem I'm talking about (just pointing out that it exists lol)
    strncpy_s(data, length, pszText, length);  // I read that putting "_TRUNCATE" as the last parameter might prevent the error about the string not being NULL-terminated... you can probably tell by now I'm a Linux guy; there are a lot of "Microsoft-isms" that don't make sense to those of us who only know standard C. :D

    *output = data;  // Could it be this?  Am I missing an "&" here?  I mean the code works (compiles and runs as expected) but I'm just not sure where/how the memory needs to be free()d lol

    // Release the lock, release the clipboard and we're done
    if (GlobalUnlock(hData) || !CloseClipboard()) {
        // NOTE: There was some other logging in here, but this area is never reached so I cut that out
        return false;
    }
    return true;
}

// And now in main()
char* clipboard = NULL;
myFunction(&clipboard);
// Again, NULL-check removed
printf("Clipboard contents: \"%s\")\r\n", clipboard);
if (clipboard != NULL) free(clipboard);    // Visual Studio MeltDown 2021.0 :D

---

Not sure what to make of this one. It's a tolerable memory leak for the project at hand, but I'd really like to see if I can find a way to patch it. If I free it in myFunction, then it's not available to main(); if I free it in main(), VS has kittens. So....? :D

PS: Sorry the whole thing is in a code tag - apparently the forum's automated checker thinks I put code somewhere else besides where I had the tags originally. Man, systems love to spew errors in my face today. :D

I don't use Windows but maybe you need to GlobalFree(hData) before the function returns. And you don't need the double pointer.
Untested:

Code:

---

#include <windows.h>
#include <stdio.h>
#include <stdio.h>
#include <string.h>
 
char* read_clipboard_text() {
    char* text = NULL;
    OpenClipboard(NULL);
    HANDLE hData = GetClipboardData(CF_TEXT);
    if (hData) {
        char* clip_text = (char*)GlobalLock(hData);
        if (clip_text) {
            size_t len = strlen(clip_text);
            text = (char*)malloc(len + 1);
            if (text)
                strncpy_s(text, len + 1, clip_text, len);
            GlobalUnlock(hData);
        }
        GlobalFree(hData);
    }
    CloseClipboard();
    return text;
}
 
int main() {
    char* text = read_clipboard_text();
    if (text) {
        printf("Clipboard contents: \"%s\")\n", text);
        free(text);
    }
    else
        printf("error\n");
    return 0;
}

---

> char* data = (char*)malloc(length);
If you're writing C, then you don't need this cast.

If your source files are the default .cpp file type that VS gives you, then you're compiling C++ code and stuck with C++ rules (including the inability to deal with void* properly).

Either
- rename your source files to be .c
- Use the /Tc flag.
/Tc, /Tp, /TC, /TP (Specify Source File Type) | Microsoft Docs

Code:

---

data[length] = NULL; 
// This was due to another weird VS error; apparently the Win32 calls don't null-terminate clipboard data automatically,
// so I'm having to do it manually here.  Could be the culprit, but that's unlikely because
// commenting this out didn't fix the problem I'm talking about (just pointing out that it exists lol)

---

Aside from the fact that NULL is not the same as '\0', this is a buffer overrun.
Eg, length is 3, you malloc(length), the valid subscripts are 0,1,2.
Subscript 3 is off in the weeds.


> if (GlobalUnlock(hData) || !CloseClipboard())
This makes no sense.
If GlobalUnlock returns true, you're not closing the clipboard.

Some of your other error paths leave resources dangling as well.

> if (clipboard != NULL) free(clipboard);
free(NULL) is actually safe to do (and does nothing), so checking is pointless.

> printf("Clipboard contents: "%s")\r\n", clipboard);
You don't need the \r.
If the OS needs a \r, then the standard runtime library will do this for you.

Wow, thanks for the quick (and thorough) responses here! You've definitely shown me a thing or two I can use to help debug this.

* I didn't know you could return a char * from a function(I thought it would go out of scope); this change alone will probably be a big help.
* You're right about myString[length] = NULL being a buffer overrun; I thought that "smelled" funny (lol, if u know what I mean) but not doing that caused VS to throw a different error that took much longer to discover; IMO any API where u have to NULL-terminate strings yourself is a recipe for disaster lol - hopefully I can avoid that altogether by rewriting this function (or at least do a memset after the malloc).
* And you're right about there being a "GlobalFree" - a quick MSDN search found it, so that is definitely something I'll want to incorporate into this function as well.
* And re-reading the docs on GlobalUnlock and CloseClipboard, I get that line about the if-statement too lol.
* As for the cast, yeah, I'm stuck with a .cpp file (for reasons I won't bore you with). I am interested though, why you say C++ doesn't handle void * properly? Just curious. :)

Anyway, yeah, like I said, thanks for all the helpful input! Visual Studio has some of the most "un-helpful" error messages I've ever seen, but getting other coders' perspective goes a long way to making sense of it.

Compilers, especially the Visual Studio compiler, generally have very precise error messages. It may be that the compiler is confused about something, or that you are confused about something, but the actual message is probably pretty clear. See if you can find a way to copy it, or just type it in by hand. Post an image if you have to.