Windows: Fix heap overflow setting native icon

[jordo]

Fixes #71697. Can probably be fixed on master as well.

[akien-mga]

We really like more descriptive commit message in a codebase that gets a few dozen commits merged everyday :)

Something like

Windows: Fix heap overflow setting native icon

Fixes #71697.

would be perfect.

[jordo]

OK will change commit msg

[jordo]

Updated commit msg

[akien-mga]

Seems like this was fixed in master already with a slightly different fix: #68631.

[bruvzg]

In master it is icon_dir = (ICONDIR *)memrealloc(icon_dir, sizeof(ICONDIR) - sizeof(ICONDIRENTRY) + icon_dir->idCount * sizeof(ICONDIRENTRY));, (see #68631) which is probably correct since ICONDIR already include the first entry.

[jordo]

Sure, that saves sizeof(ICONDIRENTRY) bytes

[jordo]

Can idCount ever be zero?

[jordo]

Part of why I guess I didn't love subtracting that part of the struct size away... Was thinking better safe than sorry, as was unsure what valid range of idCount could be without any context of how icons worked.

[bruvzg]

Can idCount ever be zero?

Technically yes (in practice no, icon file with 0 icons doesn't make sense), but it should be fine, if it's zero nothing will be written to the buffer anyway.

[jordo]

Sometimes if the spec (or whatever icon format this is working on), technically allows something it could probably be handled if it's technically possible, better to be robust imo than save a couple bytes. I've dealt with annoying .icns formats in the past which crashed a parser based on some weird assumption of practical formats and sizes.

Anywhoo should be same as master now

[jordo]

Actually, i changed my mind. I think i'm going to revert it to my original implementation. It's safer in general, and safer for future contributors who may not have context or may not work at this allocation point. Sure there are some guards there now, but I don't like that the allocation has potential to be wrong and insufficient. Maybe it could also be exploited, etc.

There just shouldn't be a possible way to allocate unsufficient memory for the size of struct required, even with checks after allocation... At the minimum it should guarantee to always allocate sizeof(ICONDIR) for safety regardless of any external multiplier (idCount).

[jordo]

I changed this commit to what is currently pushed to master... but I'm not sure what version is actually best.

When i implemented, i didn't know what valid range of idCount can be. If it can be zero, it's possibly incorrect on master as the struct won't be allocated to it's minimum size.

icon_dir = (ICONDIR *)memrealloc(icon_dir, sizeof(ICONDIR) - sizeof(ICONDIRENTRY) + icon_dir->idCount * sizeof(ICONDIRENTRY));

[akien-mga]

The CI error seems unrelated to this PR, likely Xcode got updated on GH Actions and it's now reporting this warning (which we treat as errors):

platform/osx/joypad_osx.cpp:326:3: error: 'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Werror,-Wdeprecated-declarations]
                sprintf(uid, "%08x%08x%08x%08x", OSSwapHostToBigInt32(3), OSSwapHostToBigInt32(vendor), OSSwapHostToBigInt32(product_id), OSSwapHostToBigInt32(version));
                ^
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/stdio.h:188:1: note: 'sprintf' has been explicitly marked deprecated here
__deprecated_msg("This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead.")
^
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/sys/cdefs.h:215:48: note: expanded from macro '__deprecated_msg'
        #define __deprecated_msg(_msg) __attribute__((__deprecated__(_msg)))
                                                      ^
1 error generated.

Will look into fixing this in a separate PR, then this will need a rebase to pass CI.

[akien-mga]

Will look into fixing this in a separate PR, then this will need a rebase to pass CI.

Done, once rebased this should be good to go.

[akien-mga]

This added an extra commit, we'd really prefer a rebase to keep the Git history clean.

[jordo]

arg ya, didn't see the 2 commits there... Will rebase and resubmit monday, sry!

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 3.5.2.