Fix crash trying to skip over element, doing a nullptr deref

[Lia-mon]

🍰 Pullrequest

Changing hasNext() and hasPrev() functions to avoid a nullptr dereference when iNext or iPull is null. The change preserves the current behavior of the functions (minus the crash) but the intended behavior might have been different. The way it is now it may skip an iteration step.

Proof

Obvious

Issues

Causes a crash when iterating on linked list references (3 layers of abstraction) sometimes!

Todo

Test whether there is a point of even using next() compared to nocheck_next(). Likewise with prev(). Equivalently check whether the current behavior is the correct one.

[insunaa]

hasNext shouldn't check hasNextNext tho? where are you encountering 3-layered abstractions?

[Lia-mon]

TrinityCore fix 15 years ago

It DOES check ""hasNextNext"" right now though and so I preserve the behavior in case things were coded with that strange behavior in mind.

Cause the code gets consumed when iterating through GroupReference, MapReference, GridReference etc which is 3 levels.

[insunaa]

yeah that whole thing makes no sense
this should just be

return iNext != nullptr;

imo.
as for the GroupReference, MapReference, GridReference thing as far as I can tell those are only inheritance, they don't actually cascade next()s

[Lia-mon]

It's wholly useless if that behavior is the case. Since there is no need to check at all. If hasNext() is false next() returns nullptr which is just iNext

[insunaa]

yeah, the whole thing is useless. why are we using LinkedLists

[Lia-mon]

oh god you're actually sane. Yeah that structure is not performant at all either

[insunaa]

after reading into this BS code some more I see now that there are padding elements at the start and at the end, hence the double-next check. it's still garbage tho. your PR makes sense, as much as I think linked lists should be abolished

[BlessedPinkie]

agreed linked lists are garbo

[Lia-mon]

I wouldn't really say that it needs it because of the padding elements. Also Linked Lists (and linked structures in general) are incredible when unliking / linking behavior happens intensely, but it doesn't apply to small one dimensional structures that's being used for :b

As for the hasNext behavior I don't think it skips one because of the sentinel values (head and tail-end) rember look at nocheck_next for example. Having sentinel values is good actually :b. Anyway for some of the things it's used it can be replaced (not everything!!! ) with std::vector<std::pair<T,U>>. If I have the time I will check correctness and either try to write a wrapper for it or replace some instances with a different structure

[insunaa]

For threat tables it makes sense, because those do change rapidly and require frequent linking and unlinking, but gridref or mapref? ehhhh
And even then, std::list exists and I somehow doubt 16+ year old CMaNGOS code is more efficient than a standard library implementation (tho if someone can empirically prove it one way or the other that'd be useful I think)

[Lia-mon]

The behavior checking ahead is correct, it's crashing otherwise. Im gonna assume the wise old ones put it there for a reason, I'll investigate more

[killerwife]

iNext and iPrev is enough, dont need to do != nullptr

[Lia-mon]

Im rethinking the validity of this request. Data that's inserted in the list to begin with cannot be nullptr since they'd have to get linked in which case the issue would be caught at/before insertion.

We had a crash on our server during an iteration I think it was a group reference iteration. That would mean the data in the list got invalidated externally.

I think it'd be best to let the crash take place instead of knee jerking it and figuring it where in the cleanup this would have happened. + we run custom code so it might be entirely our fault and the vanilla version may never run into this issue.

As for the the linkedlist usage it's alright, maybe TypedList that often sits on top could be a tuple instead.

[killerwife]

For reference, group lists should only be ever updated in world thread. They are one of the proverbial foot guns in the core (and all other cores). Spell system accesses it, chat does, etc and if they alter that in place instead of in world context, race conditions will occur.

[Lia-mon]

Yeah you're absolutely right. We did have a footgun, we tried to adapt the deprecated playerbots for follower dungeons and that gave us our grief.

[killerwife]

Closing this, with the caveat that if i see it and decide its needed, i will enact the change and give you credit if that arises.