[ReadWriteLibarchivePlugin] Fix memory handling (QSaveFile + ArchiveWrite)
Open, HighPublic

Description

In order to fix T10987 we need to apply the same fix as in commit b1a251ebb96b86c8cb, i.e. make LibarchivePlugin::doKill() return false at least for AddJobs.

However this change currently exposes the following crash:

==21862== Invalid read of size 8
==21862==    at 0x168510B2: archive_write_client_write (archive_write.c:385)
==21862==    by 0x1685099B: __archive_write_filter (archive_write.c:237)
==21862==    by 0x1685B385: archive_compressor_gzip_close (archive_write_add_filter_gzip.c:316)
==21862==    by 0x16850A38: __archive_write_close_filter (archive_write.c:260)
==21862==    by 0x1685158B: _archive_write_close (archive_write.c:524)
==21862==    by 0x168501D7: archive_write_close (archive_virtual.c:68)
==21862==    by 0x168517A8: _archive_write_free (archive_write.c:585)
==21862==    by 0x16850194: archive_free (archive_virtual.c:62)
==21862==    by 0x16850244: archive_write_free (archive_virtual.c:87)
==21862==    by 0x1679CABF: LibarchivePlugin::ArchiveWriteCustomDeleter::cleanup(archive*) (libarchiveplugin.h:75)
==21862==    by 0x1679ABEF: QScopedPointer<archive, LibarchivePlugin::ArchiveWriteCustomDeleter>::~QScopedPointer() (qscopedpointer.h:107)
==21862==    by 0x1679DAB6: ReadWriteLibarchivePlugin::~ReadWriteLibarchivePlugin() (readwritelibarchiveplugin.cpp:51)
==21862==  Address 0x135d5bd0 is 0 bytes inside a block of size 32 free'd
==21862==    at 0x48399AB: free (vg_replace_malloc.c:530)
==21862==    by 0x1685131A: archive_write_client_close (archive_write.c:441)
==21862==    by 0x16850A38: __archive_write_close_filter (archive_write.c:260)
==21862==    by 0x1685B4E0: archive_compressor_gzip_close (archive_write_add_filter_gzip.c:341)
==21862==    by 0x16850A38: __archive_write_close_filter (archive_write.c:260)
==21862==    by 0x1685158B: _archive_write_close (archive_write.c:524)
==21862==    by 0x168501D7: archive_write_close (archive_virtual.c:68)
==21862==    by 0x1679F0BB: ReadWriteLibarchivePlugin::finish(bool) (readwritelibarchiveplugin.cpp:412)
==21862==    by 0x1679E747: ReadWriteLibarchivePlugin::addFiles(QVector<Kerfuffle::Archive::Entry*> const&, Kerfuffle::Archive::Entry const*, Kerfuffle::CompressionOptions const&, unsigned int) (readwritelibarchiveplugin.cpp:137)
==21862==    by 0x489CEC1: Kerfuffle::AddJob::doWork() (jobs.cpp:684)
==21862==    by 0x489851C: Kerfuffle::Job::Private::run() (jobs.cpp:66)
==21862==    by 0x6857DEB: ??? (in /usr/lib/libQt5Core.so.5.13.0)
==21862==  Block was alloc'd at
==21862==    at 0x483AB65: calloc (vg_replace_malloc.c:752)
==21862==    by 0x16850D6A: archive_write_client_open (archive_write.c:301)
==21862==    by 0x168509F4: __archive_write_open_filter (archive_write.c:250)
==21862==    by 0x1685AE04: archive_compressor_gzip_open (archive_write_add_filter_gzip.c:187)
==21862==    by 0x168509F4: __archive_write_open_filter (archive_write.c:250)
==21862==    by 0x16850C7A: archive_write_open (archive_write.c:480)
==21862==    by 0x16857D38: archive_write_open_fd (archive_write_open_fd.c:75)
==21862==    by 0x1679EA7B: ReadWriteLibarchivePlugin::initializeWriter(bool, Kerfuffle::CompressionOptions const&) (readwritelibarchiveplugin.cpp:267)
==21862==    by 0x1679DD17: ReadWriteLibarchivePlugin::addFiles(QVector<Kerfuffle::Archive::Entry*> const&, Kerfuffle::Archive::Entry const*, Kerfuffle::CompressionOptions const&, unsigned int) (readwritelibarchiveplugin.cpp:67)
==21862==    by 0x489CEC1: Kerfuffle::AddJob::doWork() (jobs.cpp:684)
==21862==    by 0x489851C: Kerfuffle::Job::Private::run() (jobs.cpp:66)
==21862==    by 0x6857DEB: ??? (in /usr/lib/libQt5Core.so.5.13.0)
==21862== 
==21862== Invalid read of size 8
==21862==    at 0x168510DD: archive_write_client_write (archive_write.c:388)
==21862==    by 0x1685099B: __archive_write_filter (archive_write.c:237)
==21862==    by 0x1685B385: archive_compressor_gzip_close (archive_write_add_filter_gzip.c:316)
==21862==    by 0x16850A38: __archive_write_close_filter (archive_write.c:260)
==21862==    by 0x1685158B: _archive_write_close (archive_write.c:524)
==21862==    by 0x168501D7: archive_write_close (archive_virtual.c:68)
==21862==    by 0x168517A8: _archive_write_free (archive_write.c:585)
==21862==    by 0x16850194: archive_free (archive_virtual.c:62)
==21862==    by 0x16850244: archive_write_free (archive_virtual.c:87)
==21862==    by 0x1679CABF: LibarchivePlugin::ArchiveWriteCustomDeleter::cleanup(archive*) (libarchiveplugin.h:75)
==21862==    by 0x1679ABEF: QScopedPointer<archive, LibarchivePlugin::ArchiveWriteCustomDeleter>::~QScopedPointer() (qscopedpointer.h:107)
==21862==    by 0x1679DAB6: ReadWriteLibarchivePlugin::~ReadWriteLibarchivePlugin() (readwritelibarchiveplugin.cpp:51)
==21862==  Address 0x135d5bd0 is 0 bytes inside a block of size 32 free'd
==21862==    at 0x48399AB: free (vg_replace_malloc.c:530)
==21862==    by 0x1685131A: archive_write_client_close (archive_write.c:441)
==21862==    by 0x16850A38: __archive_write_close_filter (archive_write.c:260)
==21862==    by 0x1685B4E0: archive_compressor_gzip_close (archive_write_add_filter_gzip.c:341)
==21862==    by 0x16850A38: __archive_write_close_filter (archive_write.c:260)
==21862==    by 0x1685158B: _archive_write_close (archive_write.c:524)
==21862==    by 0x168501D7: archive_write_close (archive_virtual.c:68)
==21862==    by 0x1679F0BB: ReadWriteLibarchivePlugin::finish(bool) (readwritelibarchiveplugin.cpp:412)
==21862==    by 0x1679E747: ReadWriteLibarchivePlugin::addFiles(QVector<Kerfuffle::Archive::Entry*> const&, Kerfuffle::Archive::Entry const*, Kerfuffle::CompressionOptions const&, unsigned int) (readwritelibarchiveplugin.cpp:137)
==21862==    by 0x489CEC1: Kerfuffle::AddJob::doWork() (jobs.cpp:684)
==21862==    by 0x489851C: Kerfuffle::Job::Private::run() (jobs.cpp:66)
==21862==    by 0x6857DEB: ??? (in /usr/lib/libQt5Core.so.5.13.0)
==21862==  Block was alloc'd at
==21862==    at 0x483AB65: calloc (vg_replace_malloc.c:752)
==21862==    by 0x16850D6A: archive_write_client_open (archive_write.c:301)
==21862==    by 0x168509F4: __archive_write_open_filter (archive_write.c:250)
==21862==    by 0x1685AE04: archive_compressor_gzip_open (archive_write_add_filter_gzip.c:187)
==21862==    by 0x168509F4: __archive_write_open_filter (archive_write.c:250)
==21862==    by 0x16850C7A: archive_write_open (archive_write.c:480)
==21862==    by 0x16857D38: archive_write_open_fd (archive_write_open_fd.c:75)
==21862==    by 0x1679EA7B: ReadWriteLibarchivePlugin::initializeWriter(bool, Kerfuffle::CompressionOptions const&) (readwritelibarchiveplugin.cpp:267)
==21862==    by 0x1679DD17: ReadWriteLibarchivePlugin::addFiles(QVector<Kerfuffle::Archive::Entry*> const&, Kerfuffle::Archive::Entry const*, Kerfuffle::CompressionOptions const&, unsigned int) (readwritelibarchiveplugin.cpp:67)
==21862==    by 0x489CEC1: Kerfuffle::AddJob::doWork() (jobs.cpp:684)
==21862==    by 0x489851C: Kerfuffle::Job::Private::run() (jobs.cpp:66)
==21862==    by 0x6857DEB: ??? (in /usr/lib/libQt5Core.so.5.13.0)
==21862== 
==21862== Invalid read of size 4
==21862==    at 0x16857F14: file_write (archive_write_open_fd.c:125)
==21862==    by 0x168510E1: archive_write_client_write (archive_write.c:387)
==21862==    by 0x1685099B: __archive_write_filter (archive_write.c:237)
==21862==    by 0x1685B385: archive_compressor_gzip_close (archive_write_add_filter_gzip.c:316)
==21862==    by 0x16850A38: __archive_write_close_filter (archive_write.c:260)
==21862==    by 0x1685158B: _archive_write_close (archive_write.c:524)
==21862==    by 0x168501D7: archive_write_close (archive_virtual.c:68)
==21862==    by 0x168517A8: _archive_write_free (archive_write.c:585)
==21862==    by 0x16850194: archive_free (archive_virtual.c:62)
==21862==    by 0x16850244: archive_write_free (archive_virtual.c:87)
==21862==    by 0x1679CABF: LibarchivePlugin::ArchiveWriteCustomDeleter::cleanup(archive*) (libarchiveplugin.h:75)
==21862==    by 0x1679ABEF: QScopedPointer<archive, LibarchivePlugin::ArchiveWriteCustomDeleter>::~QScopedPointer() (qscopedpointer.h:107)
==21862==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==21862== 
==21862== 
==21862== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==21862==  Access not within mapped region at address 0x0
==21862==    at 0x16857F14: file_write (archive_write_open_fd.c:125)
==21862==    by 0x168510E1: archive_write_client_write (archive_write.c:387)
==21862==    by 0x1685099B: __archive_write_filter (archive_write.c:237)
==21862==    by 0x1685B385: archive_compressor_gzip_close (archive_write_add_filter_gzip.c:316)
==21862==    by 0x16850A38: __archive_write_close_filter (archive_write.c:260)
==21862==    by 0x1685158B: _archive_write_close (archive_write.c:524)
==21862==    by 0x168501D7: archive_write_close (archive_virtual.c:68)
==21862==    by 0x168517A8: _archive_write_free (archive_write.c:585)
==21862==    by 0x16850194: archive_free (archive_virtual.c:62)
==21862==    by 0x16850244: archive_write_free (archive_virtual.c:87)
==21862==    by 0x1679CABF: LibarchivePlugin::ArchiveWriteCustomDeleter::cleanup(archive*) (libarchiveplugin.h:75)
==21862==    by 0x1679ABEF: QScopedPointer<archive, LibarchivePlugin::ArchiveWriteCustomDeleter>::~QScopedPointer() (qscopedpointer.h:107)
`

This is possibly due to m_tempFile and m_archiveWriter being class member (since @mvlabat's GSoC refactoring), while they used to be local variables in ReadWriteLibarchivePlugin::addFiles().

This is possibly due to m_tempFile and m_archiveWriter being class member (since @mvlabat's GSoC refactoring), while they used to be local variables in ReadWriteLibarchivePlugin::addFiles().

The old version of ReadWriteLibarchivePlugin::addFiles() as reference:

bool ReadWriteLibarchivePlugin::addFiles(const QList<Archive::Entry*>& files, const CompressionOptions& options)
{
    qCDebug(ARK) << "Adding" << files.size() << "entries with CompressionOptions" << options;

    const bool creatingNewFile = !QFileInfo::exists(filename());

    m_writtenFiles.clear();

    ArchiveRead arch_reader;
    if (!creatingNewFile) {
        arch_reader.reset(archive_read_new());
        if (!arch_reader.data()) {
            emit error(i18n("The archive reader could not be initialized."));
            return false;
        }

        if (archive_read_support_filter_all(arch_reader.data()) != ARCHIVE_OK) {
            return false;
        }

        if (archive_read_support_format_all(arch_reader.data()) != ARCHIVE_OK) {
            return false;
        }

        if (archive_read_open_filename(arch_reader.data(), QFile::encodeName(filename()), 10240) != ARCHIVE_OK) {
            emit error(i18n("The source file could not be read."));
            return false;
        }
    }

    // |tempFile| needs to be created before |arch_writer| so that when we go
    // out of scope in a `return false' case ArchiveWriteCustomDeleter is
    // called before destructor of QSaveFile (ie. we call archive_write_close()
    // before close()'ing the file descriptor).
    QSaveFile tempFile(filename());
    if (!tempFile.open(QIODevice::WriteOnly | QIODevice::Unbuffered)) {
        emit error(xi18nc("@info", "Failed to create a temporary file to compress <filename>%1</filename>.", filename()));
        return false;
    }

    ArchiveWrite arch_writer(archive_write_new());
    if (!(arch_writer.data())) {
        emit error(i18n("The archive writer could not be initialized."));
        return false;
    }

    // pax_restricted is the libarchive default, let's go with that.
    archive_write_set_format_pax_restricted(arch_writer.data());

    int ret;
    bool requiresExecutable = false;
    if (creatingNewFile) {
        if (filename().right(2).toUpper() == QLatin1String("GZ")) {
            qCDebug(ARK) << "Detected gzip compression for new file";
            ret = archive_write_add_filter_gzip(arch_writer.data());
        } else if (filename().right(3).toUpper() == QLatin1String("BZ2")) {
            qCDebug(ARK) << "Detected bzip2 compression for new file";
            ret = archive_write_add_filter_bzip2(arch_writer.data());
        } else if (filename().right(2).toUpper() == QLatin1String("XZ")) {
            qCDebug(ARK) << "Detected xz compression for new file";
            ret = archive_write_add_filter_xz(arch_writer.data());
        } else if (filename().right(4).toUpper() == QLatin1String("LZMA")) {
            qCDebug(ARK) << "Detected lzma compression for new file";
            ret = archive_write_add_filter_lzma(arch_writer.data());
        } else if (filename().right(2).toUpper() == QLatin1String(".Z")) {
            qCDebug(ARK) << "Detected compress (.Z) compression for new file";
            ret = archive_write_add_filter_compress(arch_writer.data());
        } else if (filename().right(2).toUpper() == QLatin1String("LZ")) {
            qCDebug(ARK) << "Detected lzip compression for new file";
            ret = archive_write_add_filter_lzip(arch_writer.data());
        } else if (filename().right(3).toUpper() == QLatin1String("LZO")) {
            qCDebug(ARK) << "Detected lzop compression for new file";
            ret = archive_write_add_filter_lzop(arch_writer.data());
        } else if (filename().right(3).toUpper() == QLatin1String("LRZ")) {
            qCDebug(ARK) << "Detected lrzip compression for new file";
            ret = archive_write_add_filter_lrzip(arch_writer.data());
            requiresExecutable = true;
#ifdef HAVE_LIBARCHIVE_3_2_0
        } else if (filename().right(3).toUpper() == QLatin1String("LZ4")) {
            qCDebug(ARK) << "Detected lz4 compression for new file";
            ret = archive_write_add_filter_lz4(arch_writer.data());
            requiresExecutable = true;
#endif
        } else if (filename().right(3).toUpper() == QLatin1String("TAR")) {
            qCDebug(ARK) << "Detected no compression for new file (pure tar)";
            ret = archive_write_add_filter_none(arch_writer.data());
        } else {
            qCDebug(ARK) << "Falling back to gzip";
            ret = archive_write_add_filter_gzip(arch_writer.data());
        }

        // Libarchive emits a warning for lrzip due to using external executable.
        if ((requiresExecutable && ret != ARCHIVE_WARN) ||
            (!requiresExecutable && ret != ARCHIVE_OK)) {
            emit error(xi18nc("@info", "Setting the compression method failed with the following error:<nl/><message>%1</message>",
                              QLatin1String(archive_error_string(arch_writer.data()))));
            return false;
        }

        // Set compression level if passed in CompressionOptions.
        if (options.contains(QStringLiteral("CompressionLevel"))) {
            qCDebug(ARK) << "Using compression level:" << options.value(QStringLiteral("CompressionLevel")).toString();
            ret = archive_write_set_filter_option(arch_writer.data(), NULL, "compression-level", options.value(QStringLiteral("CompressionLevel")).toString().toUtf8());
            if (ret != ARCHIVE_OK) {
                qCWarning(ARK) << "Failed to set compression level";
                emit error(xi18nc("@info", "Setting the compression level failed with the following error:<nl/><message>%1</message>",
                                  QLatin1String(archive_error_string(arch_writer.data()))));
                return false;
            }
        }

    } else {
        switch (archive_filter_code(arch_reader.data(), 0)) {
        case ARCHIVE_FILTER_GZIP:
            ret = archive_write_add_filter_gzip(arch_writer.data());
            break;
        case ARCHIVE_FILTER_BZIP2:
            ret = archive_write_add_filter_bzip2(arch_writer.data());
            break;
        case ARCHIVE_FILTER_XZ:
            ret = archive_write_add_filter_xz(arch_writer.data());
            break;
        case ARCHIVE_FILTER_LZMA:
            ret = archive_write_add_filter_lzma(arch_writer.data());
            break;
        case ARCHIVE_FILTER_COMPRESS:
            ret = archive_write_add_filter_compress(arch_writer.data());
            break;
        case ARCHIVE_FILTER_LZIP:
            ret = archive_write_add_filter_lzip(arch_writer.data());
            break;
        case ARCHIVE_FILTER_LZOP:
            ret = archive_write_add_filter_lzop(arch_writer.data());
            break;
        case ARCHIVE_FILTER_LRZIP:
            ret = archive_write_add_filter_lrzip(arch_writer.data());
            requiresExecutable = true;
            break;
#ifdef HAVE_LIBARCHIVE_3_2_0
        case ARCHIVE_FILTER_LZ4:
            ret = archive_write_add_filter_lz4(arch_writer.data());
            requiresExecutable = true;
            break;
#endif
        case ARCHIVE_FILTER_NONE:
            ret = archive_write_add_filter_none(arch_writer.data());
            break;
        default:
            emit error(i18n("The compression type '%1' is not supported by Ark.",
                            QLatin1String(archive_filter_name(arch_reader.data(), 0))));
            return false;
        }

        // Libarchive emits a warning for lrzip due to using external executable.
        if ((requiresExecutable && ret != ARCHIVE_WARN) ||
            (!requiresExecutable && ret != ARCHIVE_OK)) {
            emit error(xi18nc("@info", "Setting the compression method failed with the following error:<nl/><message>%1</message>",
                              QLatin1String(archive_error_string(arch_writer.data()))));
            return false;
        }
    }

    ret = archive_write_open_fd(arch_writer.data(), tempFile.handle());
    if (ret != ARCHIVE_OK) {
        emit error(xi18nc("@info", "Opening the archive for writing failed with the following error:<nl/><message>%1</message>",
                          QLatin1String(archive_error_string(arch_writer.data()))));
        return false;
    }

    // First write the new files.
    qCDebug(ARK) << "Writing new entries";
    int no_entries = 0;
    foreach(Archive::Entry *selectedFile, files) {

        if (m_abortOperation) {
            break;
        }

        if (!writeFile(selectedFile->property("fullPath").toString(), arch_writer.data())) {
            return false;
        }
        no_entries++;

        // For directories, write all subfiles/folders.
        const QString &fullPath = selectedFile->property("fullPath").toString();
        if (QFileInfo(fullPath).isDir()) {
            QDirIterator it(fullPath,
                            QDir::AllEntries | QDir::Readable |
                            QDir::Hidden | QDir::NoDotAndDotDot,
                            QDirIterator::Subdirectories);

            while (!m_abortOperation && it.hasNext()) {
                QString path = it.next();

                if ((it.fileName() == QLatin1String("..")) ||
                    (it.fileName() == QLatin1String("."))) {
                    continue;
                }

                const bool isRealDir = it.fileInfo().isDir() && !it.fileInfo().isSymLink();

                if (isRealDir) {
                    path.append(QLatin1Char('/'));
                }

                if (!writeFile(path, arch_writer.data())) {
                    return false;
                }
                no_entries++;
            }
        }
    }
    qCDebug(ARK) << "Added" << no_entries << "new entries to archive";

    struct archive_entry *entry;

    // If we have old archive entries.
    if (!creatingNewFile) {

        qCDebug(ARK) << "Copying any old entries";
        no_entries = 0;

        // Copy old entries from previous archive to new archive.
        while (!m_abortOperation && (archive_read_next_header(arch_reader.data(), &entry) == ARCHIVE_OK)) {

            const QString entryName = QFile::decodeName(archive_entry_pathname(entry));

            if (m_writtenFiles.contains(entryName)) {
                archive_read_data_skip(arch_reader.data());
                qCDebug(ARK) << entryName << "is already present in the new archive, skipping.";
                continue;
            }

            const int returnCode = archive_write_header(arch_writer.data(), entry);

            switch (returnCode) {
            case ARCHIVE_OK:
                // If the whole archive is extracted and the total filesize is
                // available, we use partial progress.
                copyData(QLatin1String(archive_entry_pathname(entry)), arch_reader.data(), arch_writer.data(), false);
                no_entries++;
                break;
            case ARCHIVE_FAILED:
            case ARCHIVE_FATAL:
                qCCritical(ARK) << "archive_write_header() has returned" << returnCode
                                << "with errno" << archive_errno(arch_writer.data());
                emit error(xi18nc("@info", "Compression failed while processing:<nl/>"
                                  "<filename>%1</filename><nl/><nl/>Operation aborted.", entryName));
                QFile::remove(tempFile.fileName());
                return false;
            default:
                qCWarning(ARK) << "archive_writer_header() has returned" << returnCode
                               << "which will be ignored.";
                break;
            }
            archive_entry_clear(entry);
        }
        qCDebug(ARK) << "Added" << no_entries << "old entries to archive";
    }

    m_abortOperation = false;

    // In the success case, we need to manually close the archive_writer before
    // calling QSaveFile::commit(), otherwise the latter will close() the
    // file descriptor archive_writer is still working on.
    // TODO: We need to abstract this code better so that we only deal with one
    // object that manages both QSaveFile and ArchiveWriter.
    archive_write_close(arch_writer.data());
    tempFile.commit();

    return true;
}
elvisangelaccio removed a subscriber: mvlabat.
elvisangelaccio moved this task from Backlog to In progress on the Ark board.Jun 2 2019, 8:15 AM

The actual stacktrace here is fixed by dfbbc9561e, but df024e3d13 should prevent this crash from happening in the first place.

elvisangelaccio moved this task from In progress to Under review on the Ark board.Jun 2 2019, 8:53 AM