Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#24908: Validate saved files and alert user #24911

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/engraving/infrastructure/mscwriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,24 @@ using namespace muse;
using namespace muse::io;
using namespace mu::engraving;

// FileCorruptor
FileCorruptor::FileCorruptor(const path_t& filePath)
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
: File(filePath)
{
}

size_t FileCorruptor::writeData(const uint8_t* data, size_t len)
{
// Ignore the actual data and write only zeros
Q_UNUSED(data)
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
uint8_t* zerosData = new uint8_t[len];
for (size_t i = 0; i < len; i++) {
zerosData[i] = 0;
}
return File::writeData(zerosData, len);
}

// MscWriter
MscWriter::MscWriter(const Params& params)
: m_params(params)
{
Expand Down Expand Up @@ -266,7 +284,11 @@ Ret MscWriter::ZipFileWriter::open(io::IODevice* device, const path_t& filePath)
{
m_device = device;
if (!m_device) {
m_device = new File(filePath);
// Create a corrupted file if the filename contains " - CORRUPTED.mscz".
// This is only for DEV/QA testing and will not be included in a release.
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
m_device = filePath.toQString().contains(" - CORRUPTED.mscz")
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
? new FileCorruptor(filePath)
: new File(filePath);
m_selfDeviceOwner = true;
}

Expand Down
12 changes: 12 additions & 0 deletions src/engraving/infrastructure/mscwriter.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "types/ret.h"
#include "io/path.h"
#include "io/iodevice.h"
#include "io/file.h"
#include "mscio.h"

namespace muse {
Expand All @@ -34,6 +35,17 @@ class TextStream;
}

namespace mu::engraving {
// FileCorruptor: a class that writes only zeros to a file no matter what it is passed
class FileCorruptor : public muse::io::File
{
public:
FileCorruptor(const muse::io::path_t& filePath);

protected:
size_t writeData(const uint8_t* data, size_t len) override;
};

// MscWriter
class MscWriter
{
public:
Expand Down
55 changes: 54 additions & 1 deletion src/project/internal/notationproject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -641,8 +641,24 @@ Ret NotationProject::doSave(const muse::io::path_t& path, engraving::MscIoMode i
LOGW() << ret.toString();
}
} else {
Ret ret = fileSystem()->move(savePath, targetContainerPath, true);
Ret ret = checkSavedFileForCorruption(ioMode, savePath, targetMainFileName.toQString());
if (!ret) {
if (ret.code() == (int)Err::CorruptionUponSavingError) {
ret.setData("corruptedAfterMove", false);
}
return ret;
}

ret = fileSystem()->move(savePath, targetContainerPath, true);
if (!ret) {
return ret;
}

ret = checkSavedFileForCorruption(ioMode, targetContainerPath, targetMainFileName.toQString());
RomanPudashkin marked this conversation as resolved.
Show resolved Hide resolved
if (!ret) {
if (ret.code() == (int)Err::CorruptionUponSavingError) {
ret.setData("corruptedAfterMove", true);
}
return ret;
}
}
Expand Down Expand Up @@ -779,6 +795,43 @@ Ret NotationProject::saveSelectionOnScore(const muse::io::path_t& path)
return ret;
}

Ret NotationProject::checkSavedFileForCorruption(engraving::MscIoMode ioMode, QString path, QString scoreFileName)
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
{
if (ioMode != MscIoMode::Zip) {
return muse::make_ok();
}

// TODO: Execute this on Windows only?

MscReader::Params params;
params.filePath = path;
params.mainFileName = scoreFileName;
params.mode = MscIoMode::Zip;

MscReader msczReader(params);
Ret ret = msczReader.open();
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
if (!ret) {
return Ret(static_cast<int>(Err::CorruptionUponSavingError),
muse::mtrc("project/save", "File “%1” could not be opened. %2")
.arg(path)
.arg(String(ret.toString().c_str()))
.toStdString());
}

// Try to extract the main score file.
ByteArray scoreFile = msczReader.readScoreFile();
msczReader.close();

if (scoreFile.empty()) {
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
return Ret(static_cast<int>(Err::CorruptionUponSavingError),
muse::mtrc("project/save", "File “%1” is corrupted or damaged: the score file could not be read.")
.arg(path)
.toStdString());
}

return muse::make_ok();
}

Ret NotationProject::exportProject(const muse::io::path_t& path, const std::string& suffix)
{
TRACEFUNC;
Expand Down
1 change: 1 addition & 0 deletions src/project/internal/notationproject.h
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ class NotationProject : public INotationProject, public muse::Injectable, public
muse::Ret doSave(const muse::io::path_t& path, engraving::MscIoMode ioMode, bool generateBackup = true, bool createThumbnail = true);
muse::Ret makeCurrentFileAsBackup();
muse::Ret writeProject(engraving::MscWriter& msczWriter, bool onlySelection, bool createThumbnail = true);
muse::Ret checkSavedFileForCorruption(engraving::MscIoMode ioMode, QString path, QString scoreFileName);

void listenIfNeedSaveChanges();
void markAsSaved(const muse::io::path_t& path);
Expand Down
29 changes: 28 additions & 1 deletion src/project/internal/projectactionscontroller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -930,7 +930,11 @@ bool ProjectActionsController::saveProjectLocally(const muse::io::path_t& filePa

if (!ret) {
LOGE() << ret.toString();
warnScoreCouldnotBeSaved(ret);
if (ret.code() == (int)Err::CorruptionUponSavingError) {
warnScoreCorruptAfterSave(ret);
} else {
warnScoreCouldnotBeSaved(ret);
}
return false;
}

Expand Down Expand Up @@ -1577,6 +1581,29 @@ void ProjectActionsController::warnScoreCouldnotBeSaved(const std::string& error
interactive()->warning(muse::trc("project/save", "Your score could not be saved"), errorText);
}

void ProjectActionsController::warnScoreCorruptAfterSave(const Ret& ret)
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
{
std::string errMessage = ret.toString();

std::any corruptedAfterMove = ret.data("corruptedAfterMove");
std::string originalFileCorruptedMessage = corruptedAfterMove.has_value() && std::any_cast<bool>(corruptedAfterMove)
? "Unfortunately your original file has become corrupt.\n\n"
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
: "";

std::string message = muse::qtrc("project/save",
"An error occurred while saving your file. "
"This may be a one-off error. Please retry saving the file in order not to lose your latest changes.\n\n"
"If it keeps happening, try saving the file to a different location or to MuseScore.com. "
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
" You could also try saving the file as an uncompressed folder or XML.\n\n"
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
"%1"
"For help please use the \"Support and bug reports\" forum at https://musescore.org/en/forum\n\n"
krasko78 marked this conversation as resolved.
Show resolved Hide resolved
"Error: %2")
.arg(originalFileCorruptedMessage.c_str())
.arg(errMessage.c_str())
.toStdString();
interactive()->warning(muse::trc("project/save", "Your score could not be saved"), message);
}

void ProjectActionsController::revertCorruptedScoreToLastSaved()
{
TRACEFUNC;
Expand Down
1 change: 1 addition & 0 deletions src/project/internal/projectactionscontroller.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ class ProjectActionsController : public IProjectFilesController, public muse::mi

void warnScoreCouldnotBeSaved(const muse::Ret& ret);
void warnScoreCouldnotBeSaved(const std::string& errorText);
void warnScoreCorruptAfterSave(const muse::Ret& ret);

void revertCorruptedScoreToLastSaved();

Expand Down
1 change: 1 addition & 0 deletions src/project/projecterrors.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ enum class Err {
NoPartsError,
CorruptionError,
CorruptionUponOpenningError,
CorruptionUponSavingError, // do we need this or can we use CorruptionError instead?

FileOpenError,
InvalidCloudScoreId,
Expand Down
Loading