## Brief overview of PR changes/additions
Comprehensive fix for package installation crashes (issue #8154) that
builds upon the defensive checks from @vadi2's PR #8181.
This PR addresses the root causes of the crash by:
1. **Blocking concurrent operations** - Prevents package installation
while profile save is in progress
2. **Deferring event handlers** - Moves sysInstall event raising to
after package import completes
3. **Improving error recovery** - Better validation in LuaInterface when
searching for Lua tables
## Motivation for adding to Mudlet
Fixes#8154 - crash when installing packages with variables in certain
profiles
The crash occurred because:
- Variables were being loaded into Lua state while event handlers were
executing
- Profile save and package installation could run concurrently
- Event handlers triggered during XMLimport could corrupt Lua stack
state
## Other info
This PR incorporates and builds upon @vadi2's excellent diagnostic work
in PR #8181. The defensive checks from that PR are included, plus
additional fixes for the underlying race conditions.
**Changes in this PR:**
### From vadi2's PR #8181:
- Added Lua stack validation in `LuaInterface::loadValue()` before
calling `lua_gettable()`
- Added null check for Lua state in `callEventHandler()`
- Fixed stack index bug (using `-1` instead of `1` for error checking)
- Added stack cleanup on error paths
- Added emergency stop mode check
### Additional fixes in this PR:
- Added `currentlySavingProfile()` check at the start of
`Host::installPackage()` to prevent concurrent operations
- Deferred `sysInstall`, `sysInstallPackage`, and related event handlers
to execute after package import completes using `QTimer::singleShot()`
- Added validation and error reporting when searching for tables with
`index=0` in `LuaInterface::loadValue()`
Co-authored-by: @vadi2
---------
Co-authored-by: Vadim Peretokin <vadi2@users.noreply.github.com>
Co-authored-by: Vadim Peretokin <vperetokin@hey.com>
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Fix memory leak in variables view
#### Motivation for adding to Mudlet
Should not have any
#### Other info (issues closed, discussion etc)
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Move all variable initializers to header classes
#### Motivation for adding to Mudlet
Cleaner code, and a better example for LLMs to follow when generating
code
#### Other info (issues closed, discussion etc)
No functional changed are expected
---------
Co-authored-by: Vadim Peretokin <vadi2@users.noreply.github.com>
Co-authored-by: Kebap <kebap_spam@gmx.net>
#### Brief overview of PR changes/additions
For Windows builds modify the `#include` lines for Lua header files to
specify the 5.1 version. Also accommodate some changes in our CI build
environment:
* For some reason (maybe because of a more modern linker) we need to
specify the original PCRE library with `-lpcre` rather than `-lpcre-1` -
the exact cause of this is not clear but thanks to @jmckisson for
finding it (and using it in his attempt to solve the same problems this
PR is doing).
* It seems the Window building is now being done in the `C:` drive
rather than the previous `D:` one, so a tweak to clean the colon
containing file-system root specifier to the alternative that
MSYS2+Mingw-w64 uses which instead uses a (POSIX) `/` root directory
followed by a single lower-case letter to specify the drive needs to be
extended to handle both drives. This is because the scripts use `rsync`
and that treats any `:` as the separator between host and path and gets
confused when it sees a "Windows" path containing it!
#### Motivation for adding to Mudlet
The default version - and the one needed for some packages like Luarocks
is a 5.4 one - and that includes header files in the "default" `include`
directory. So the headers that get pulled in are the wrong ones, which
fail to work as they are not compatible with Lua 5.1; to get the 5.1
instead I believe we need to explicitly include the version specific
sub-directory in the `#include` lines.
Other tweaks are also needed "to get things working nowadays."
#### Other info (issues closed, discussion etc)
This should be simpler to do than what is being attempted by #7841.
---------
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Mark read-only variables as const - that is, not intended to change
after they've been declared
#### Motivation for adding to Mudlet
So we don't change them by accident later on, and also to clearly state
intentions for the variables
#### Other info (issues closed, discussion etc)
This is also recommended by the [C++ Core
Guidelines](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es25-declare-an-object-const-or-constexpr-unless-you-want-to-modify-its-value-later-on)
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Improve names of variables that are too short and not meaningful
#### Motivation for adding to Mudlet
So developers have an easier time working with Mudlets code - it's
crucial to be able to attract and retain folks working on the project
for its success!
#### Other info (issues closed, discussion etc)
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Improve names of variables that are too short and not meaningful
#### Motivation for adding to Mudlet
So developers have an easier time working with Mudlets code - it's
crucial to be able to attract and retain folks working on the project
for its success!
#### Other info (issues closed, discussion etc)
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
If you created a table right at _G with a key that was a number, the
variables view would crash.
#### Motivation for adding to Mudlet
Fix https://github.com/Mudlet/Mudlet/issues/6119
#### Other info (issues closed, discussion etc)
Fixed by adding a case alongside existing support for string keys to
support number keys as well. Function, table, userdata, etc keys are not
supported but they won't crash either.
The `Host*` is never actually used inside the `LuaInterface` class, only in
a non-default constructor. So it does not need to be retained as a class
member.
As the default is never used either we can also mark the above constructor
as `explicit`...
Also `mMaxAreaId` is declared in the `XMLimport` class but although a use
had been considered it is not currently used for anything.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Also removes unused:
* (bool) Host::mAutoReconnect
* (QString) Host::mBufferIncomingData
* (QPushButton*) Host::uninstallButton
* (QListWidget*) Host::packageList
* (QListWidget*) Host::moduleList
* (QPushButton*) Host::moduleUninstallButton
* (QPushButton*) Host::moduleInstallButton
* (bool) TConsole::mWindowIsHidden
Also add some initialisers, particularly for pointers and booleans.
I have spotted in a lot of the new TMxpXxx classes that:
* the include guards are not right at the top of the file
* private members and methods are declared first in the header files rather
than the public ones which are odd as the latter are more likely to be of
interest to anyone wishing to examine the details of the API that the
latter document
* in header files that include another header file the inclusion of that
file is done twice - once before and once after the class members and
methods are documented. Given the use of inclusion guards the second is
pointless - and if there were not such guards the second inclusion would
(I believe) break things...
I have only included fixing this in the TMxpSendTagHandler class in this
PR but there are others that also need the above matters attended to...
For me this reduces the warning count from 662 to 502.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
The latter has been present since Qt 4.7 and even on some Windows OSes
where it may be a 32-Bit value that can overflow after nearly 50 days
when the lower quality TickCounter clock is used as a fallback instead of
the PerformanceCounter one this is a better bet than the QTime based one
that will overflow (wrap) after 24 hours and will be affected by
Summer-Time changes and user/system adjustment of the system clock.
Also:
* rename three elements related to timing the network latency, their
original names were somewhat ambiguous:
* (QLineEdit*) TConsole::networkLatency
==> TConsole::mpLineEdit_networkLatency
* (double) cTelnet::networkLatency
==> cTelnet::networkLatencyTime
* (QTime) cTelnet::networkLatencyTime
==> (QElapsedTimer) cTelnet::networkLatencyTimer
* rename some other elements in a comparable way:
* (QTime) cTelnet::timeOffset
==> (QElapsedTimer) cTelnet::mRecordingChunkTimer
* (QTime) cTelnet::mConnectionTime
==> (QElapsedTimer) cTelnet::mConnectionTimer
* (int) cTelnet::lastTimeOffset
==> (int) cTelnet::mRecordLastChunkMSecTimeOffset
* simplify a C string array access - there is no need to use the address
operator AND an index when referring to the start of a C array, i.e. for
char buffer[datalen]: '&buffer[0]' is simply 'buffer' !
* the display of the network latency (if available) and the system
processing time is a UI feature but it was not being put through the
translation system, this commit now allows for that to happen.
This should remove 12 warnings (on my Linux Qt 5.14.2 system).
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
* lua_error()+return 1 => return lua_error()
* Missed fix-ups from the last cleanup patch
* Flip conditions to remove return-error-else antipatterns
* also fix TForkedProcess
* Remove QString::fromUtf8 calls
They're superfluous and make the code less readable.
Classed as "High Impact":
CID Type Detail
1492499 "Uninitialized scalar variable (UNINIT) "5. uninit_use: Using
uninitialized value error."
1485860 "No virtual destructor" "A1. dtor_in_derived:
Class `XMLimport` has a compiler-generated destructor. It is non-empty
because of its field `mpHost`. A pointer to class `XMLimport` is upcast to
class `QXmlStreamReader` which doesn't have a virtual destructor."
Classed as "Medium Impact":
1492834 "Uninitialized scalar field (UNINIT_CTOR)" "2. uninit_member:
Non-static class member `mIsEndTag` is not initialized in this constructor
nor in any functions that it calls.
4. uninit_member: Non-static class member `mIsEmptyTag` is not initialized
in this constructor nor in any functions that it calls.
6. uninit_member: Non-static class member `mReadingAttrValue` is not
initialized in this constructor nor in any functions that it calls.
8. uninit_member: Non-static class member `mOpeningQuote` is not
initialized in this constructor nor in any functions that it calls."
1488910 "Uninitialized scalar field (UNINIT_CTOR)" "2. uninit_member:
Non-static class member `mPlayerRoomStyle` is not initialized in this
constructor nor in any functions that it calls."
"4. uninit_member: Non-static class member
`mPlayerRoomOuterDiameterPercentage` is not initialized in this
constructor nor in any functions that it calls."
"6. uninit_member: Non-static class member
`mPlayerRoomInnerDiameterPercentage` is not initialized in this
constructor nor in any functions that it calls."
1478854 "Uninitialized pointer field (UNINIT_CTOR)" "4. uninit_member:
Non-static class member `mpOutOfBandDataIncomingCodec` is not initialized
in this constructor nor in any functions that it calls."
1468478 "Unchecked return value (CHECKED_RETURN)" "10. check_return:
Calling `luaL_loadstring` without checking return value (as is done
elsewhere 17 out of 21 times)."
1468477 "Unchecked return value (CHECKED_RETURN)" "14. check_return:
Calling `luaL_loadstring` without checking return value (as is done
elsewhere 17 out of 21 times)."
1468474 "Unchecked return value (CHECKED_RETURN)" "16. check_return:
Calling `luaL_loadstring` without checking return value (as is done
elsewhere 17 out of 21 times)." - x 2
1468468 "Logically dead code (DEADCODE)" "dead_error_line:
Execution cannot reach this statement: `return 1;`"
1415097 "Dereference null return value (NULL_RETURNS)" "8. dereference:
Dereferencing timer, which is known to be `nullptr`"
1415092 "Identical code for different branches (IDENTICAL_BRANCHES)"
"identical_branches: The same code is executed regardless of whether
`areaExit` is true, because the 'then' and 'else' branches are identical.
Should one of the branches be modified, or the entire 'if' statement
replaced?"
1415023 "Dereference null return value (NULL_RETURNS)" "26. dereference:
Dereferencing a pointer that might be `nullptr` `pR->name` when calling
`QString`.
1414989 "Explicit null dereferenced (FORWARD_NULL)" "81. var_deref_op:
Dereferencing null pointer
`this->originalExits.value(dirCode, TExit * const(NULL))`." x 11
1414977 "Logically dead code (DEADCODE)" "dead_error_line:
Execution cannot reach this statement: `return false;`."
Also removed unused:
* (int) cTelnet::curX & curY,
* (double) cTelnet::networkLatencyMin & networkLatencyMax
* (QMutex) TimerUnit::mTimerUnitLock
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
This is a squash and merge of five commits:
* Allow setting variables to non-Latin values via GUI
* Allow deleting non-Latin named variables
* Updated rest of LuaInterface to i18n
* Wrap lua_tostring calls in utf8
* Applied review comments
clang-format has an option to sort header #include lines in source code
files - however the `#include "pre_guard.h"` and `#include "post_guard.h"`
includes are position sensitive - they must wrap around Qt's own classes'
includes in our files and they must not be tidied-up/rearranged.
This commit adds the necessary comments that commands clang tools to not
touch these lines.
We had previously done this on the XMLimport class as a test case but
during recent major branch merging operations one of these comments was
lost from the XMLimport.cpp file!
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
* BugFix: fix crash on profile auto-load (bug 1624720)
The uninitialised TVar::parent member in the default constructor is used
in the profile auto-load (but not the normal one apparently) and was the
cause for segment violations. In fixing THIS I noted that several other
members of that Class {and worryingly, quite a few *other* classes
throughout the Mudlet application!!!) were not being initialised to
sensible defaults either. This commit addresses the TVar class, but ONLY
that Class...!
In checking also found that (void) TVar::setNewName( QString ) is not used
{only the two argument (void) TVar::setNewName( QString, int ) that takes
a Lua Type number as a second argument IS used} so commented it out. Also
found that (int) TVar::nvType and (QString) TVar::nValue members are not
used so removed them as well.
Also fixed a (subtle ?) bug in local functor TVarLessThan(TVar *, TVar *) which
tested the number value of the "name" of each argument when converted to a
number without checking that the "name" could be converted to a number.
Consequently, when a non-number name was encountered it would convert to
zero and THAT zero value would be compared to the other one. If BOTH
arguments were not numbers then the caller would ALWAYS be told the first
argument was less than the second argument whereas the wanted result was
a String comparison of "lessnessness".
Refactor TVar class methods that return a "bool" type to actually return
such a thing rather than (int)(0) for false or, more suspiciously, (int)(1)
for true - though subsequent research revealed that that is how C++ does
it, (as a C ex-pat I just think of anything that isn't zero is true)! This
affects:
* (bool) TVar::setValue( const QString )
* (bool) TVar::setValue( const QString, const int )
* (bool) TVar::setValueType( const int )
Similarly, where an argument was an int type but was being used in a
binary/boolean manner I've changed it to the latter type. This affected:
* (QList<TVar *>) TVar::getChildren( const bool )
This affects what are now:
* (void) VarUnit::buildVarTree( QTreeWidgetItem *, TVar *, bool )
* (bool) XMLexport::writeHost( Host * )
* (bool) XMLexport::writeVariable( TVar *, LuaInterface *, VarUnit * )
* (void) dlgTriggerEditor::slot_search_triggers( const QString )
* (int) dlgTriggerEditor::canRecast( QTreeWidgetItem *, int, int )
* (void) dlgTriggerEditor::recurseVariablesDown( TVar *, QList<TVar *> &,
bool)
* (void) LuaInterface::getAllChildren( TVar *, QList<TVar *> *)
Tidies up code layout of TVar implementation to current coding style.
Adds const where possible to TVar class method parameters.
__This commit is a squashed down one as some later ones of the original commits were reverted directly on GitHub in the reverse of their application - this comment contains the text for the remaining parts and hopefully still make sense!__
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
This also allows for memory leak checking on MSVC by having include
surrounding Qt headers, which break if you have a macro define new
to be something which tracks line information.
