added a check to verify itemList validity (empty)

[R4ven47]

Added check to verify whether itemList is empty or not

[Dutchman101]

If this is the way to go, doesn't it also need to be reflected in CFastList.h's ListContains?

But, is it the way to go? I get that this PR is to fix your MTA Linux server's intermittent segfault crashes (the ones you asked for support with on Dev and MTA discord under your account name "Snowfall"), but did you ever wonder how the itemList comes to be empty in the first place: when it happens, where does it come from? MTA server has various uses of ListContains, and to name a few (there's really a bunch more) and give you an idea of what would need to be figured out: ListContains @ CPedManager::Exists, ListContains @ CTeamManager::Exists, etc. Can you find any scenario that's subject to a bug or race condition which may end up with a bad or missing entry being sent to ListContains? Is there a better place to put sanity checks earlier on? Also, is there a logical correlation to a certain (flawed logic) use of scripts on your server that can cause such a condition due to forwarding/triggering on something that doesn't exist? Of course, it would still expose an MTA bug while doing so. As far i know, no other server owners came forward with this segmentation fault crash.

Sometimes it's better to find and address the problem, than hiding it (this can lead to undefined behavior and other issues down the chain), this isn't my cup of tea but I wouldn't exclude the possibility code review looks at these aspects and it may not go through without closer analysis of what's actually going on here.

[R4ven47]

If this is the way to go, doesn't it also need to be reflected in CFastList.h's ListContains?

But, is it the way to go? I get that this PR is to fix your MTA Linux server's intermittent segfault crashes (the ones you asked for support with on Dev and MTA discord under your account name "Snowfall"), but did you ever wonder how the itemList comes to be empty in the first place: when it happens, where does it come from? MTA server has various uses of ListContains, and to name a few (there's really a bunch more) and give you an idea of what would need to be figured out: ListContains @ CPedManager::Exists, ListContains @ CTeamManager::Exists, etc. Can you find any scenario that's subject to a bug or race condition which may end up with a bad or missing entry being sent to ListContains? Also, is there a logical correlation to a certain (flawed logic) use of scripts on your server that can cause such a condition due to forwarding/triggering on something that doesn't exist? Of course, not to say it exposes an MTA bug while doing so. As far i know, no other server owners came forward with this segmentation fault crash.

Sometimes it's better to find and address the problem, than hiding it (this can lead to undefined behavior and other issues down the chain), this isn't my cup of tea but I wouldn't exclude the possibility code review looks at these aspects and it may not go through without closer analysis of what's actually going on here.

The issue is not specific to my MTA server on its own, how that comes to be the case on ARM64 arch for another individual is beyond me, what I can assume is that on Ubuntu 16.04 x64 and ARM64 distros, something external may be deallocating the list early on (the LuaVM garbage collector maybe?), or some system interference on memory, or when a script is unloaded from the LuaVM the list gets cleared (but what if it clears it intermittently)? What led me to this is the fact that both me and the other person are getting the same segmentation fault error due to m_TimerList being compared to pLuaTimer in CLuaTimerManager.cpp: CLuaTimerManager::RemoveTimer(CLuaTimer* pLuaTimer). Both servers crash when there's a heavy load on them due to this error. My server at least ran fine for years until this issue popped out of nowhere.

Regardless, I don't see how adding an additional check is a problem in all cases. Thorough analysis can be done, assuming that the rest of contributors/staff would be down to tracking the issue, since as I said elsewhere, it's pretty recent and only became an issue after 1.6.

[Dutchman101]

I see, fair enough.. the priority now is to get you (and anyone else that may be affected) a crash-free build, further analysis with the help of those details, e.g timer related, you just provided, can be done afterwards if anyone is interested.

Linux and arm server builds with this fix will appear in under an hour, thanks

[tederis]

What does it change? What is the purpose of this? From the code perspective it doesn't change the behaviour. The emptiness of lists is an absolutely valid situation when you call contains method or compare iterators. If the origin of a segfault is within the list memory corruption then empty method will lead to a segfault as well.

[Dutchman101]

Well, unless you can answer tederis on that one, it will be reverted in the next 24hrs

[R4ven47]

What does it change? What is the purpose of this? From the code perspective it doesn't change the behaviour. The emptiness of lists is an absolutely valid situation when you call contains method or compare iterators. If the origin of a segfault is within the list memory corruption then empty method will lead to a segfault as well.

Correct me if I'm wrong. As far as I'm aware, internally speaking, an empty list is also a null one, i.e one with a null beginning (size = 0, nullptr) which is a scenario empty() explicitly handles (which assuming the contains method handled, a segfault wouldn't be thrown). If this still results in a segfault (which so far hasn't been the case) I'd be more than happy to let you guys know to revert, if not, I don't see how these additional checks will be anything more than sanity checks in the code. I reiterate and say it would be more interesting to collectivity look further into what I mentioned above.

[Pieter-Dewachter]

False, a nullptr is only apllicable for pointers (hence the name). It means a pointer is pointing towardsa null address, which indiates that pointer is not pointing to anything.

A list (actually more correct would be a container class) can be something pointed to by a pointer, but it's not a pointer by itself. C++ explicitely requires that empty containers satisfy the following: list.begin() == list.end(). For this case, it means the for-loop will not be executed and that the extra check is redundant (maybe the compiler will even optimize it away).

I assume the confusion comes from the dereference operator that can be used on iterators, giving a reference to the element in that position of the container. This is actually done internally by operator overloading, it doesn't mean the iterator is actually just a plain pointer (as it does have methods for example, which pointers don't have).

[R4ven47]

False, a nullptr is only apllicable for pointers (hence the name). It means a pointer is pointing towardsa null address, which indiates that pointer is not pointing to anything.

A list (actually more correct would be a container class) can be something pointed to by a pointer, but it's not a pointer by itself. C++ explicitely requires that empty containers satisfy the following: list.begin() == list.end(). For this case, it means the for-loop will not be executed and that the extra check is redundant (maybe the compiler will even optimize it away).

I assume the confusion comes from the dereference operator that can be used on iterators, giving a reference to the element in that position of the container. This is actually done internally by operator overloading, it doesn't mean the iterator is actually just a plain pointer (as it does have methods for example, which pointers don't have).

Thank you for the reply. Indeed, a list is a container class. However, internally (memory level) it is implemented as a doubly-linked list in which every element is linked forward to the next and back to the previous one, which all goes back to the head being NULL or not. I am very much aware of the condition to satisfy for an empty container, but a segmentation fault was thrown after evaluating the ListContains statements (hence the very initial empty() comparison I added).

[tederis]

What does it change? What is the purpose of this? From the code perspective it doesn't change the behaviour. The emptiness of lists is an absolutely valid situation when you call contains method or compare iterators. If the origin of a segfault is within the list memory corruption then empty method will lead to a segfault as well.

Correct me if I'm wrong. As far as I'm aware, internally speaking, an empty list is also a null one, i.e one with a null beginning (size = 0, nullptr) which is a scenario empty() explicitly handles (which assuming the contains method handled, a segfault wouldn't be thrown). If this still results in a segfault (which so far hasn't been the case) I'd be more than happy to let you guys know to revert, if not, I don't see how these additional checks will be anything more than sanity checks in the code. I reiterate and say it would be more interesting to collectivity look further into what I mentioned above.

Lists(as well as maps on which CFastList is based) in STL are all initialized with sentinel iterators. That means that even in case of emptiness STL containers will always return valid iterators(the sentinel ones). ::contains is also internally based on the comparison of iterators. Thus, unless there is a memory corruption STL containers will aways keep invariants and be valid. On the other side, if the memory corruption occures additional checks wont help. I am not saying that this PR is bad. Just seems that it does nothing.

[R4ven47]

What does it change? What is the purpose of this? From the code perspective it doesn't change the behaviour. The emptiness of lists is an absolutely valid situation when you call contains method or compare iterators. If the origin of a segfault is within the list memory corruption then empty method will lead to a segfault as well.

Correct me if I'm wrong. As far as I'm aware, internally speaking, an empty list is also a null one, i.e one with a null beginning (size = 0, nullptr) which is a scenario empty() explicitly handles (which assuming the contains method handled, a segfault wouldn't be thrown). If this still results in a segfault (which so far hasn't been the case) I'd be more than happy to let you guys know to revert, if not, I don't see how these additional checks will be anything more than sanity checks in the code. I reiterate and say it would be more interesting to collectivity look further into what I mentioned above.

Lists(as well as maps on which CFastList is based) in STL are all initialized with sentinel iterators. That means that even in case of emptiness STL containers will always return valid iterators(the sentinel ones). ::contains is also internally based on the comparison of iterators. Thus, unless there is a memory corruption STL containers will aways keep invariants and be valid. On the other side, if the memory corruption occures additional checks wont help. I am not saying that this PR is bad. Just seems that it does nothing.

Thank you for replying once again. I will look into tracking the main issue as soon as I get a free time window, I am not entirely sure if this behavior is due to ARM64/Ubuntu 16.04 deprecated support for new C++ libs, but I'll make sure to verify that as well.