Skip to content

Commit

Permalink
Fix: prevent crashes when trying to open a non-zip file (Mudlet#7501)
Browse files Browse the repository at this point in the history 
#### Brief overview of PR changes/additions
Don't try and dereference a nullptr returned from a failed
`zip_open(...)` call. Instead use the recommended method that constructs
a `zip_error_t` instance from the `int` error code set by that failed
`zip_open(...)`.

#### Motivation for adding to Mudlet
Prevent the crashes reported by @gesslar in Mudlet#7495 and on Discord.

#### Other info (issues closed, discussion etc)
Also:
* clean up better after a failure in `(void)
Host::updateModuleZips(...)` which was failing to call
`zip_discard(zip*)` should a `zip_close(zip*)` fail.
* Move a `char[]` buffer in `(bool) mudlet::zip(...)` closer to where it
is used.
* Add error message production (to the OS command line) to a stage in
`(void) Host::updateModuleZips(...)`.

This should close Mudlet#7495.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
  • Loading branch information
@SlySven
SlySven authored Oct 26, 2024
1 parent 7ea173a commit eef6941
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 25 deletions.
34 changes: 30 additions & 4 deletions src/Host.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@
#include <memory>
#include "post_guard.h"

// We are now using code that won't work with really old versions of libzip;
// some of the error handling was improved in 1.0 . Unfortunately libzip 1.7.0
// (and one or two other recent versions) forgot to include the version defines
// and thus broke a test depending on them:
#if defined(LIBZIP_VERSION_MAJOR) && (LIBZIP_VERSION_MAJOR < 1)
#error Mudlet requires a version of libzip of at least 1.0
#endif

using namespace std::chrono;

stopWatch::stopWatch()
Expand Down Expand Up @@ -733,6 +741,17 @@ void Host::updateModuleZips(const QString& zipName, const QString& moduleName)
int err = 0;
zipFile = zip_open(zipName.toStdString().c_str(), ZIP_CREATE, &err);
if (!zipFile) {
zip_error_t zipError;
zip_error_init_with_code(&zipError, err);
/*: This zipError message is shown when the libzip library code is unable
* to open the file that was to be the end result of the export process.
* As this may be an existing file anywhere in the computer's
* file-system(s) it is possible that permissions on the directory or an
* existing file that is to be overwritten may be a source of problems
* here.
*/
qWarning().noquote().nospace() << "Host::updateModuleZips(\"" << zipName << "\", \"" << moduleName << "\") WARNING - failed to open module to update it, error: \"" << zip_error_strerror(&zipError) << "\"";
zip_error_fini(&zipError);
return;
}
const QDir packageDir = QDir(packagePathName);
Expand All @@ -750,13 +769,20 @@ void Host::updateModuleZips(const QString& zipName, const QString& moduleName)
err = zip_file_add(zipFile, qsl("%1.xml").arg(moduleName).toUtf8().constData(), s, ZIP_FL_ENC_UTF_8 | ZIP_FL_OVERWRITE);

if (zipFile) {
// This is the point at which the data is written out to the archive,
// might take a bit of time:
err = zip_close(zipFile);
}

if (mudlet::smDebugMode && err == -1) {
//: This error message will appear when a module is saved as package but cannot be done for some reason.
TDebug(QColor(Qt::white), QColor(Qt::red)) << tr("Failed to save \"%1\" to module \"%2\". Error message was: \"%3\".")
.arg(moduleName, zipName, zip_strerror(zipFile));
if (err == -1) {
if (mudlet::smDebugMode && err == -1) {
//: This error message will appear when a module is saved as package but cannot be done for some reason.
TDebug(QColor(Qt::white), QColor(Qt::red)) << tr("Failed to save \"%1\" to module \"%2\". Error message was: \"%3\".")
.arg(moduleName, zipName, zip_strerror(zipFile));
}
// Properly dispose of things after failing to zip_close(...) the
// archive:
zip_discard(zipFile);
}
}

Expand Down
33 changes: 19 additions & 14 deletions src/dlgPackageExporter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,12 @@
#include <QMimeData>
#include "post_guard.h"

// We are now using code that won't work with really old versions of libzip:
// Unfortunately libzip 1.70 forgot to include these defines and thus broke the
// original tests:
#if defined(LIBZIP_VERSION_MAJOR) && defined(LIBZIP_VERSION_MINOR) && (LIBZIP_VERSION_MAJOR < 1) && (LIBZIP_VERSION_MINOR < 11)
#error Mudlet requires a version of libzip of at least 0.11
// We are now using code that won't work with really old versions of libzip;
// some of the error handling was improved in 1.0 . Unfortunately libzip 1.7.0
// (and one or two other recent versions) forgot to include the version defines
// and thus broke a test depending on them:
#if defined(LIBZIP_VERSION_MAJOR) && (LIBZIP_VERSION_MAJOR < 1)
#error Mudlet requires a version of libzip of at least 1.0
#endif

dlgPackageExporter::dlgPackageExporter(QWidget *parent, Host* pHost)
Expand Down Expand Up @@ -895,10 +896,12 @@ std::pair<bool, QString> dlgPackageExporter::zipPackage(const QString& stagingDi
if (!archive) {
zip_error_t zipError;
zip_error_init_with_code(&zipError, ze);
/*:
This zipError message is shown when the libzip library code is unable to open the file that was to be the end result of the export process. As this may be an existing
file anywhere
in the computer's file-system(s) it is possible that permissions on the directory or an existing file that is to be overwritten may be a source of problems here.
/*: This zipError message is shown when the libzip library code is unable
* to open the file that was to be the end result of the export process.
* As this may be an existing file anywhere in the computer's
* file-system(s) it is possible that permissions on the directory or an
* existing file that is to be overwritten may be a source of problems
* here.
*/
const QString errMsg = tr("Failed to open package file. Error is: \"%1\".")
.arg(zip_error_strerror(&zipError));
Expand Down Expand Up @@ -1058,10 +1061,12 @@ std::pair<bool, QString> dlgPackageExporter::zipPackage(const QString& stagingDi
return {false, tr("Export cancelled.")};
}

/*:
This error message is displayed at the final stage of exporting a package when all the sourced files are finally put into the archive. Unfortunately this may be
the point at which something breaks because a problem was not spotted/detected in the process earlier...
*/
/*: This error message is displayed at the final stage of exporting
* a package when all the sourced files are finally put into the
* archive. Unfortunately this may be the point at which something
* breaks because a problem was not spotted/detected in the process
* earlier...
*/
const QString errorMsg = tr("Failed to zip up the package. Error is: \"%1\".").arg(zipError);
zip_discard(archive);
// In libzip 0.11 a function was added to clean up
Expand All @@ -1070,11 +1075,11 @@ std::pair<bool, QString> dlgPackageExporter::zipPackage(const QString& stagingDi
// - before that version the memory just leaked away...
return {false, errorMsg};
}

} else {
zip_discard(archive);
}


return {isOk, error};
}

Expand Down
22 changes: 15 additions & 7 deletions src/mudlet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@
#include <QSettings>
#endif

// We are now using code that won't work with really old versions of libzip;
// some of the error handling was improved in 1.0 . Unfortunately libzip 1.7.0
// (and one or two other recent versions) forgot to include the version defines
// and thus broke a test depending on them:
#if defined(LIBZIP_VERSION_MAJOR) && (LIBZIP_VERSION_MAJOR < 1)
#error Mudlet requires a version of libzip of at least 1.0
#endif

#if defined(Q_OS_MACOS)
// wrap in namespace since `Collection` defined in these headers will clash with Boost
namespace coreMacOS {
Expand Down Expand Up @@ -3398,13 +3406,12 @@ bool mudlet::unzip(const QString& archivePath, const QString& destination, const
struct zip_stat zs;
struct zip_file* zf;
zip_uint64_t bytesRead = 0;
char buf[4096]; // Was 100 but that seems unduly stingy...!
zip* archive = zip_open(archivePath.toStdString().c_str(), 0, &err);
if (err) {
zip_error_t *error = zip_get_error(archive);
qWarning().noquote().nospace() << "mudlet::unzip(\"" << archivePath << "\", \"" << destination << "\", \"" << tmpDir.absolutePath() << "\") Warning - " << zip_error_strerror(error);
zip_error_fini(error);
zip_discard(archive);
zip* archive = zip_open(archivePath.toUtf8().constData(), 0, &err);
if (!archive) {
zip_error_t error;
zip_error_init_with_code(&error, err);
qWarning().noquote().nospace() << "mudlet::unzip(\"" << archivePath << "\", \"" << destination << "\", \"" << tmpDir.absolutePath() << "\") WARNING - failed to unzip file, error: \"" << zip_error_strerror(&error) << "\"";
zip_error_fini(&error);
return false;
}

Expand Down Expand Up @@ -3472,6 +3479,7 @@ bool mudlet::unzip(const QString& archivePath, const QString& destination, const
bytesRead = 0;
zip_uint64_t const bytesExpected = zs.size;
while (bytesRead < bytesExpected && fd.error() == QFileDevice::NoError) {
char buf[4096]; // Was 100 but that seems unduly stingy...!
zip_int64_t const len = zip_fread(zf, buf, sizeof(buf));
if (len < 0) {
fd.close();
Expand Down

0 comments on commit eef6941

Please sign in to comment.