[NOSQUASH] Remove the unnecessary sort in CNullDriver::addTexture

[Desour]

TL;DR: A performance fix for faster client startup.

I've done some little profiling on client connect. Here's a flame graph showing Game::startup of a session where it took about 40 seconds to connect to a server:

The std::__introsort_loop is the sort of CNullDriver::Textures that we do for each texture. This sorting is unnecessary.

CNullDriver::Textures holds an irr::core::array that is possibly sorted by texture name. Textures are accessed via binary search (see irrArray.h), which always sorts anyway. The comment that was already there also explains this.
CNullDriver::Textures is also not used by any child class, FYI.

Benchmarks (of Client::afterContentReceived, and Game::startup in parenthesis):
(I haven't run these often. Values are rounded.)

lightweight scenario (singleplayer mtg + mesecons):
before: 1.1 s (2.0 s)
after: 0.9 (1.8)

heavy scenario (some server):
before: 35.9 s (44.4 s)
after: 5.0 s (12.7 s)

[sfan5]

I don't think we use this but this sort call can be removed too:

[Desour]

renameTexture is actually broken, if I'm not wrong, because Textures is not flagged as unsorted before the Textures.sort() call.
I've removed the function now, together with another unused not useful one.

[appgurueu]

(For the future) could this be refactored to use an unordered map?

[sfan5]

As I just found out here texture names are apparently not an one-to-one mapping to textures 🤔
So, maybe.

[Desour]

If they are not supposed to be unique, we could still use an unordered multimap.
It would make sense imo, I just didn't do it because I wanted the change to be simple. (Also, if I change the header, I have to recompile half of minetest.)

[BeverlyCode]

Test_User/hax fixed this exact problem four months prior to this being posted, in a private fork we maintain, we also have numerous other fixes that still remain including an RCE exploit.

[SmallJoker]

The Irrlicht situation, visualized:

@BeverlyCode Please feel free to forward such patches to upstream Irrlicht so that all forks could benefit from it. Fixes in private forks do not help anyone else.

[appgurueu]

@BeverlyCode Please feel free to forward such patches to upstream Irrlicht so that all forks could benefit from it. Fixes in private forks do not help anyone else.

(Or just publicize your currently "private" fork, or parts of it, so we can take a look).

Also feel free to responsibly disclose any vulnerabilities to us and/or Irrlicht as applicable.

[BeverlyCode]

Sure I have messaged him about it, he doesn't like to use GitHub, but he's fairly active in the community I think for people the reach out to.

I don't deal with Minetest much anymore but he pointed out to me today this was finally fixed in the main branch as he checks the main branch updates now and then.

[appgurueu]

I looked through Irrlicht's commit history a bit, and it seemed to me like any (or at least most) recent security improvements (in loaders) stemmed from IrrlichtMT. Could you point to a specific commit, or as said, disclose the vulnerability to us? Are you sure there is presently a vulnerability this vulnerability is currently present in IrrlichtMT?

[BeverlyCode]

I looked through Irrlicht's commit history a bit, and it seemed to me like any recent security improvements (in loaders) stemmed from IrrlichtMT. Could you point to a specific commit, or as said, disclose the vulnerability to us? Are you sure there is presently a vulnerability in IrrlichtMT?

I made sure a few people got the core dumps and such, I can't remember what exactly it was other than Test_User found a memory leak where remote code could potentially write out of bounds. I don't think it was specifically Irrlicht code.

I have tried to get a lot of Test_Users improvements committed to mainstream branches in the past, like I ensured his improvements to the Unified Inventory server load time got merged, that's a notable one.

Usually I do my best at the time to get the word out, in some way at the time.

Edit: oh yes I remember now I think it was the entity system, the same bug that was causing pipeworks to crash. there is a way you can trick it into overflowing memory if you know how, combined with a pet name tag or any formspec input that is not validated which seems to be a lot, arb code could be spammed into the entity list. Something like this, it was a while ago now. Test_User will know the full details or have a log of it to refer to if anyone wants the full details on that.

[BeverlyCode]

Just had some messages from Test_User concerning his non-pushed changes to Minetest:

1. Regarding that RCE (just now reading the full comments on mt pull 233): unsure which was RCE-y, but there was at least the near-impossible-to-exploit inv set via pipeworks using stale pointers meaning there's potential for it to overwrite some lua code to arbitrary stuff but the lack of control over where it goes makes it pretty hard to exploit.
2. The uninitialised block map thingy that could have leaked an admin acc's passwords and allowed "rce" via lua and a hashcrack. (edit test_user told me it's a vmanip fix)
3. mapgen was the main fix that I don't think would get accepted by them, since the huge perf hit; maybe they'd make it togglable but no multithreading mapgen in my basic fix for it in exchange for not having stuff get the occasional partially rolled back and desynced (this fixed the issue of Digtron digging into caves and then mapgen updating and erasing bits of digtron - things like this i think)
4. One entity desync patch, but I think that got upstreamed seperately (upstreamed by the guys who I found it with) and no vulns in it

As a server operator I'd be inclined to say not to share the mapgen fix as it would give my server an advantage others do not, and in such a highly competitive server listing systems obviously this is completely reasonable.
(also having pipeworks not crash all the time is another advantages I would be un-inclined to share but Luckily I am not Test_User so maybe he's more sharing than me)

But I don't really care anymore. I don't think the players care about it that much either.

There really is little incentive as a server operator to tell anyone about any fixes to the code, but I do every now and then anyway, it's never at my benefit and honestly usually at my loss, primarily its a loss of my time but also any communication with the Minetest community involves... a great deal of wasted time. (no I don't fancy pushing Test_User's changes to the main branch all the time sorry)

There are many bugs in the code which will go unsolved for years, even though there have been multiple issues concerning them for years and everyone has been made aware they exist. They won't go solved for two primary reasons:

1. Most people don't believe they exist and assume you are trolling. (a community issue, if the community has dropped to that quality it needs to be fixed as fighting with people over it is an insane loss of time, although again part of the problem is many people in the community have too much spare time and seem to achieve very little with it)
2. No one knows how to fix them or probably hasn't even gotten that far to even find them.

and to be honest I don't really like any of you. 🤷

[sfan5]

Our primary concern are security-relevant issues in the engine or IrrlichtMt.
You can disclose them as written in our security policy and they will absolutely get looked at.

and to be honest I don't really like any of you. 🤷

That is regrettable but it was your choice to comment here.