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

[core, SQL] Refactor to use mariadb-connector-cpp and introduce new db namespace #4601

Merged
merged 49 commits into from
Apr 1, 2024

Conversation

zach2good
Copy link
Contributor

@zach2good zach2good commented Oct 12, 2023

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

https://github.com/LandSandBoat/server/wiki/Database-Library-Upgrade

Steps to test these changes

Test as normal (I changed some things and invalidated all my old testing, so we're starting from the beginning!):

  • All xi_world functionality (HTTP server, conquest system, daily tally)
  • All xi_search functionality (/sea all, AH, and anything else that goes through it)
  • Regular xi_map functionality continues to work, including staying up for 12+ hours (to make sure TryPing hasn't broken)
  • Send a /tell to yourself
  • /yell
  • Do some quests
  • Fight some monsters
  • Log in, log out
  • Create a character
  • Delete a character
  • /sea all
  • Teleport a full party somewhere
  • Look up some stuff on AH
  • Look at someone's seacom message
  • Hang around and wait for conquest stuff to happen
  • Set up and use the HTTP API
  • Form a party, chat, disband
  • Form a party with an added trust, chat, disband
  • Form an alliance, chat, disband
  • Talk in a Linkshell
  • Make sure RPC still works (SendLuaFuncStringToZone in mog_tablets: !exec xi.mogTablet.hideAllTablets())
  • Form a party, chat, etc. across multiple processes
  • Console system: gm Testo 5
  • Before/after Tracy profiling of regular queries and some query-heavy routines
  • Before/after Tracy profiling of a couple of test spots that will use prepared statements
  • Confirm everything works with task_system:
        // clang-format off
        {
            ts::task_system ts;
            for (auto zoneId : zonesOnThisProcess)
            {
                ts.schedule([zoneId]()
                {
                    TracyZoneScoped;
                    auto* PZone = g_PZoneList[zoneId];

                    auto Query = "SELECT \
image

@zach2good zach2good force-pushed the update_mariadb_win_libs branch 2 times, most recently from c7d76c3 to 0b8c87e Compare October 12, 2023 10:42
@zach2good zach2good changed the title [chore] Windows: Update MariaDB C connector Refactor SQL connection to use Prepared Statements Oct 12, 2023
@zach2good zach2good force-pushed the update_mariadb_win_libs branch 4 times, most recently from a422f08 to 2182ddf Compare December 17, 2023 22:22
@zach2good zach2good changed the title Refactor SQL connection to use Prepared Statements [core, SQL] Refactor to use mariadb-connector-cpp and new db namespace Dec 17, 2023
@zach2good zach2good force-pushed the update_mariadb_win_libs branch 3 times, most recently from 87942b1 to 2fbb29f Compare December 17, 2023 22:38
@zach2good zach2good added the enhancement New feature request label Dec 17, 2023
@zach2good zach2good changed the title [core, SQL] Refactor to use mariadb-connector-cpp and new db namespace [core, SQL] Refactor to use mariadb-connector-cpp and introduce new db namespace Dec 17, 2023
@zach2good
Copy link
Contributor Author

Evening discoveries: both C++-style mariadb connectors are very much 1-indexed. 😷

@zach2good zach2good force-pushed the update_mariadb_win_libs branch 3 times, most recently from 240de61 to 2fbdee3 Compare December 18, 2023 23:08
@LandSandBoat LandSandBoat deleted a comment from github-actions bot Dec 18, 2023
@zach2good zach2good force-pushed the update_mariadb_win_libs branch 2 times, most recently from 41568db to 35f0d9d Compare December 19, 2023 09:47
@zach2good zach2good marked this pull request as ready for review December 19, 2023 11:11
@zach2good
Copy link
Contributor Author

Added some basic locking to make this use case safe (it would crash out otherwise):

    auto thVec = std::vector<std::jthread>{};
    for (int i = 0; i < 32; ++i)
    {
        thVec.emplace_back(std::jthread(
        [&]()
        {
            auto rset = db::query("SELECT * FROM char_inventory;");
            if (rset && rset->rowsCount())
            while (rset->next())
            {
                ShowInfo("%u", rset->getUInt("charid"));
            }
            std::this_thread::sleep_for(std::chrono::milliseconds(xirand::GetRandomNumber(100, 500)));
        }));
    }

@zach2good zach2good force-pushed the update_mariadb_win_libs branch 5 times, most recently from bd545ad to b2a79f3 Compare December 22, 2023 22:37
If we want to collect together all of the definitions, we can do that as a lookup to strings later
@zach2good
Copy link
Contributor Author

TODO: Triple check that if a simple query fails, it'll return nullptr as the rset

@zach2good
Copy link
Contributor Author

TODO: Run through BLU logic to make sure this query works:

    void LoadSetSpells(CCharEntity* PChar)
    {
        TracyZoneScoped;

        if (PChar->GetMJob() == JOB_BLU || PChar->GetSJob() == JOB_BLU)
        {
            const char* Query = "SELECT set_blue_spells FROM "
                                "chars WHERE charid = (?);";

            auto rset = db::preparedStmt(Query, PChar->id);
            if (rset && rset->rowsCount() && rset->next())
            {
                db::extractFromBlob(rset, "set_blue_spells", PChar->m_SetBlueSpells);
            }

@zach2good
Copy link
Contributor Author

TODO: Ensure that blobs stored in the old system can be loaded by the new system, and that everything works from a before/after standpoint (logging in as BLU, messing around with spells, and zoning is sufficient to check this)

return result;
};

return replaceAll(query, { "--", "/*", "*/", ";" }, "");
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this supposed to replace "bad" characters with escaped ones? ' -> \' and so on so that things like chat, search comments, bazaar comments, etc can still include them, but they also wreck injection?

Speaking of, ' is missing from this list.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

also the autostranslate junk people can put in bazaar and seacom, thats not even human visible characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this supposed to replace "bad" characters with escaped ones? ' -> ' and so on so that things like chat, search comments, bazaar comments, etc can still include them, but they also wreck injection?

Speaking of, ' is missing from this list.

My blob encoding handles escaping characters and things, this sanitise routine is specifically to make super ultra mega sure we're removing any and all possible vectors for SQL injection, even if the c connector, cpp connector and other mechanisms all promise they stop it on our behalf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

autotranslate, bazaar, seacon etc. are all handled as blobs, which I now have acting equivalently to the c-connector, and it's now obvious what it's doing.

This routine is for the cases where plain text is pulled out of incoming packets and used as strings, rather than blobs for the db or the client

Copy link
Member

Choose a reason for hiding this comment

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

are gm/chat audits blobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be, easy test thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do this in later PRs, I already have evidence of a blob being read from the old system, rewritten and reread correctly

@zach2good zach2good force-pushed the update_mariadb_win_libs branch 2 times, most recently from a2b4b62 to 77f1d4d Compare April 1, 2024 12:25
@zach2good
Copy link
Contributor Author

All my TODOs are addressed, all my testing is done, all my comments and thoughts are addressed. Will finish the article, test TryPing and then ready for merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants