Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove blowfish.dll dependency #1566

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

ZivDero
Copy link
Contributor

@ZivDero ZivDero commented Mar 8, 2025

By default, the game's executable attempts to load blowfish.dll using COM and uses it for encryption and decryptio, which causes ceratin issues on modern operating systems. This PR removes the game's dependency on blowfish.dll.

This PR include code by CCHyper/tomsons26 from Vinifera which is derived from C&C Remastered Collection sources and is licensed under GPLv3, and as such, requires relicensing Phobos under GPL.

Copy link

github-actions bot commented Mar 8, 2025

Nightly build for this pull request:

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

@TaranDahl TaranDahl added Interaction Something related to interaction with other extension, program etc. ⚙️T2 T2 maintainer review is sufficient labels Mar 9, 2025
Copy link
Contributor

@chaserli chaserli left a comment

Choose a reason for hiding this comment

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

  • It should be discussed whether straws and pipes should be put into yrpp, and if yrpp should change its license
  • iirc BF in BlowStraw and BlowPipe were something like BlowfishEnginePtr * and had a COM interface. So I assume you're redefining these 2 classes, then you might just use normal smart pointers.

@ZivDero
Copy link
Contributor Author

ZivDero commented Mar 9, 2025

  • It should be discussed whether straws and pipes should be put into yrpp, and if yrpp should change its license
  • iirc BF in BlowStraw and BlowPipe were something like BlowfishEnginePtr * and had a COM interface. So I assume you're redefining these 2 classes, then you might just use normal smart pointers.
  1. IMO they should not; YRpp contain bindings, not reimplementations, and this isn't even present in gamemd.exe
  2. What's the point? This is vanilla code, which works fine. I see no significant reasons to alter it in any way. ALso, it might cause issues, because the memry is being deleted by the game's code, but by our come, so we need to use the game's memory allocator.

@chaserli
Copy link
Contributor

chaserli commented Mar 9, 2025

  1. What's the point? This is vanilla code, which works fine. I see no significant reasons to alter it in any way. ALso, it might cause issues, because the memry is being deleted by the game's code, but by our come, so we need to use the game's memory allocator.

If my analysis were correct, BF in blowstraw/blowpipe were something like BlowfishEngine** not BlowfishEngine*, and the size was 0x1099. Allocation/deallocation were handled by blowfish.dll instead of gamemd. Here you've taken over the entire process so you can use local allocator.

@ZivDero
Copy link
Contributor Author

ZivDero commented Mar 9, 2025

  1. What's the point? This is vanilla code, which works fine. I see no significant reasons to alter it in any way. ALso, it might cause issues, because the memry is being deleted by the game's code, but by our come, so we need to use the game's memory allocator.

If my analysis were correct, BF in blowstraw/blowpipe were something like BlowfishEngine** not BlowfishEngine*, and the size was 0x1099. Allocation/deallocation were handled by blowfish.dll instead of gamemd. Here you've taken over the entire process so you can use local allocator.

Actually, that's not the case, which is something I learnt experimentally. Since the code was borrowed from Vinifera, it originally contained new, which I was forced to replace with GameCreate after the game crashed in vanilla unpactched code attempting to free memory.

Let's just leave legacy code as it is, there is 0 need to any anything about it.

class FakeBlowfishEngine
{
public:
BlowfishEngine* CTOR_Proxy() { return new (reinterpret_cast<BlowfishEngine*>(this)) BlowfishEngine; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think keeping this makes any sense. It shouldn't be called anyway and is not correct since you get rid of the com interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As-is, it is indeed inconsequential. An alternative route we could take is NOT replacing BlowPipe and BlowStraw - then the contrstructor WOULD get called. Removing the com interface by itself is not a problem - all we would need to do is a few byte patches to change the amount of memory allocated from the small amount needed fo the COM proxy to the actual amount required for the class.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just meant this part can be commented out because you didn't take the other route. btw you allocated with GameCreate but still deallocated with dll delete in blowpipe and blowstraw

/**
* Skip loading BLOWFISH.DLL
*/
DEFINE_JUMP(LJMP, 0x6BC33A, 0x6BC425);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the win32 registry code at 6bc425 be skipped as well? I tried and there seems to be no side effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would you skip this? It is completely unrelated to Blowfish.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just asking, think you might know

@ZivDero
Copy link
Contributor Author

ZivDero commented Mar 27, 2025

I've gone ahead and
a) Removed the replacements for BlowStraw and BlowPipe, and instead patched the places where BlowfishEngine is allocated.
b) Replaced blowfish.h and blowfish.cpp with those from the Red Alert sources, with minimal adjustments.

Note for @chaserli : It is absolutely valid to replace the COM proxy with the actual class, the only thing that needs to be taken into account is the memory allocation. Having done what I did, it works just fine.

@ZivDero ZivDero requested review from chaserli and Metadorius March 27, 2025 15:37
Copy link
Member

@Metadorius Metadorius left a comment

Choose a reason for hiding this comment

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

Where's the actual license change? :P

@ZivDero
Copy link
Contributor Author

ZivDero commented Apr 10, 2025

Where's the actual license change? :P

#1567

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Interaction Something related to interaction with other extension, program etc. ⚙️T2 T2 maintainer review is sufficient
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants