Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Path validation fixes #214

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Path validation fixes #214

merged 1 commit into from
Sep 26, 2024

Conversation

MiranDMC
Copy link
Collaborator

No description provided.

@MiranDMC MiranDMC requested a review from x87 September 25, 2024 12:39
@x87 x87 changed the title Mod loader fixes Path validation fixes Sep 25, 2024
// ModLoader's hooks require relative paths in LoadLibrary to work
FilepathRemoveParent(buff, CLEO_GetGameDirectory());

ptr = LoadLibrary(buff.c_str());
Copy link

Choose a reason for hiding this comment

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

why not

        OPCODE_READ_PARAM_FILEPATH(path);

        HMODULE ptr = LoadLibrary(path);
        if (ptr != nullptr)
        {
            m_libraries.insert(ptr);
        }
        OPCODE_WRITE_PARAM_PTR(ptr);
        OPCODE_CONDITION_RESULT(ptr != nullptr);
        return OR_CONTINUE;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about LoadLibrary("kernel32.dll") ?

Copy link

Choose a reason for hiding this comment

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

I assume the path shouldn't change and remain "kernel32.dll"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you change work dir with 0A99 it will be added at the beginning then. That function did not used path reading macro on purpose.

Copy link

Choose a reason for hiding this comment

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

Wdym? If workdir is within game directory it will stripped from the path.

Copy link
Collaborator Author

@MiranDMC MiranDMC Sep 26, 2024

Choose a reason for hiding this comment

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

ML seems unable to install its hooks if library passed to LoadLibrary is not relative path. It happens when loading cleo plugins too.
I think CLEO_ResolvePath will still resolve some paths as absolute. Like virtual "cleo:"

Copy link

Choose a reason for hiding this comment

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

ML has nothing to do with virtual/absolute paths. What script was reported not working to make this change?

Copy link

Choose a reason for hiding this comment

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

Can you revert the change to 0AA2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ML has nothing to do with virtual/absolute paths. What script was reported not working to make this change?

https://github.com/cleolibrary/CLEO5/blob/master/source/CScriptEngine.cpp#L682
Perhaps if changed it will become dead loop in some cases, as resolved path will still be seen as not resolved.

Copy link

Choose a reason for hiding this comment

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

If we want to make virtual path ML-compatible (i.g. relative to game dir), then we should do it in ResolvePath. stream_custom_script "cleo:\1.cs" and load_library "cleo:\1.dll" should work the same.

@MiranDMC MiranDMC merged commit 08188a5 into master Sep 26, 2024
1 check passed
@MiranDMC MiranDMC deleted the ModLoader_fixes branch September 26, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants