Skip to content

Conversation

@rickbrew
Copy link
Contributor

@rickbrew rickbrew commented Jun 1, 2025

The documentation indicates that you should use GlobalFree on the value stored at the address given by pProfile.

In other words,

BYTE* pProfile = NULL;
CreateProfileFromLogColorSpaceW(..., &pProfile);
GlobalFree((HGLOBAL)pProfile);

This is wrong, it is a memory leak. If you do this 65,536 times in a row then this method will start returning ERROR_NOT_ENOUGH_MEMORY. Apparently that's the maximum number of HGLOBALs you can have.

The correct pattern is this:

BYTE* pProfile = NULL;
CreateProfileFromLogColorSpaceW(..., &pProfile);
HGLOBAL hProfile = GlobalHandle(pProfile);
GlobalFree(hProfile);

I ran into this issue while working on my desktop app. I had a bug in a caching layer that was causing my color profile caching to not work, so it was recreating the profile on every call into the cache. After using the app for a few minutes it would crash because I convert Win32 errors into .NET exceptions.

cc @Sergio0694

…om memory leak

The documentation indicates that you should use `GlobalFree` on the value stored at the address given by `pProfile`.

In other words, 

```c
BYTE* pProfile = NULL;
CreateProfileFromLogColorSpaceW(..., &pProfile);
GlobalFree((HGLOBAL)pProfile);
```

This is wrong, it is a memory leak. If you do this 65,536 times in a row then this method will start returning `ERROR_NOT_ENOUGH_MEMORY`. Apparently that's the maximum number of `HGLOBAL`s you can have.

The correct pattern is this:

```c
BYTE* pProfile = NULL;
CreateProfileFromLogColorSpaceW(..., &pProfile);
HGLOBAL hProfile = GlobalHandle(pProfile);
GlobalFree(hProfile);
```

I ran into this issue while working on my desktop app. I had a bug in a caching layer that was causing my color profile caching to not work, so it was recreating the profile on every call into the cache. After using the app for a few minutes it would crash because I convert Win32 errors into .NET exceptions.

cc @Sergio0694
@prmerger-automator
Copy link

@rickbrew : Thanks for your contribution! The author(s) and reviewer(s) have been notified to review your proposed change.

@rickbrew rickbrew changed the title Update CreateColorProfileFromLogColorSpaceW to steer people away from a memory leak Update CreateColorProfileFromLogColorSpace to steer people away from a memory leak Jun 1, 2025
@HBelusca
Copy link
Contributor

Note that the "wrong" case you mention, could be correct only if internally, they were using GlobalAlloc with the GMEM_FIXED flag. (See also https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-globallock#remarks ).
Your observation of the memory leak, if one doesn't use the GlobalLock call (as your solution suggests), seems to indicate they didn't use the GMEM_FIXED flag... This is my understanding, at least.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes documentation that was instructing developers to use an incorrect memory management pattern that causes memory leaks when using CreateProfileFromLogColorSpace functions. The documentation previously suggested calling GlobalFree directly on the buffer pointer, but the correct approach is to first use GlobalHandle to get the handle, then free that handle.

  • Updates the remarks section in both ASCII and Unicode function documentation to describe the correct memory management pattern
  • Adds references to the GlobalHandle function in the "See also" sections

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
nf-icm-createprofilefromlogcolorspacew.md Updated Unicode version documentation with correct memory management instructions
nf-icm-createprofilefromlogcolorspacea.md Updated ASCII version documentation with correct memory management instructions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants