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

CAL may delete partitions during disk-probe #1044

Open
philmmanjaro opened this Issue Oct 29, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@philmmanjaro
Member

philmmanjaro commented Oct 29, 2018

Describe the bug
During the last phase of the v17.1 development cycle we got a forums post claiming CAL may delete existing partition content without a user intention to do so. Seems I mislead it with #882 back then. There was a partition mounted on /tmp/Calamares-TqYRNd. With some talk about it a log was provided. Notable line was:

[0]: QTemporaryDir: Unable to remove "/tmp/Calamares-TqYRNd" most likely due to the presence of read-only files.

Back then I tested further and only found /tmp/calamares-root-5aoq_pz0 mounts, which is normal during installation phase. Since I couldn't reproduce the issue back then v17.1 got released with CAL v3.1.11-7.


In February a second user reported a similar issue with CAL v3.1.12-3. In that case it was mounted on /tmp/Calamares-EmsHcv. In the log we find a similar report:

[0]: QTemporaryDir: Unable to remove "/tmp/Calamares-EmsHcv" most likely due to the presence of read-only files.


Now in October a third user reports the same issue. In this case it got mounted in /tmp/calamares-SxYYhk and got the same line with CAL v3.2.2.1-3:

[0]: QTemporaryDir: Unable to remove "/tmp/calamares-SxYYhk" most likely due to the presence of read-only files.

Also this drive was formatted with ext4. This you can see on the session log. So I went ahead and did a full lengthy regression test with v18.0. Beside a small coding issue on my end I didn't found any regression. However, we seem to found the issue. Probably because it failed to umount and then QTemporaryDir deleted the contents automatically when the function returned. By default a mount is with rw rights. Auto-remove is on by default for: QTemporaryDir mountsDir; - doc.qt

Offending code exists since July-16 and got introduced with f2f5a18.

To Reproduce
Steps to reproduce the behavior:

  1. Start any ISO with CAL 3.1-beta2 or higher
  2. Start CAL and sit back
  3. In rare cases QTemporaryDir may delete one of your existing partition during disk probe
  4. You may close CAL at Welcome screen and try to restore your data if you have a backup

Expected behavior
Disk probe without the risk of loosing data.

Screenshots and Logs
9f2dc88accc253354d9d540b9af17cd3023d9544_1_690x388

Logs: Dec-17, Feb-18, Oct-18

Additional context
You may find the whole story in our forum. Current v18.0 ISOs can be downloaded from here: X, K, G

@stikonas

This comment has been minimized.

Contributor

stikonas commented Oct 29, 2018

Almost surely Calamares issue, never saw it with Partition Manager. @adriaandegroot can you look at it first?

@philmmanjaro

This comment has been minimized.

Member

philmmanjaro commented Oct 29, 2018

Maybe this might fix it ...

@philmmanjaro

This comment has been minimized.

Member

philmmanjaro commented Oct 29, 2018

@adriaandegroot: do we need during disk probing write access to the drives? Seems when it fails to unmount data gets erased due to the functions of QTemporaryDir. Create random dir -> mount -> unmount -> destroy dir. We may miss some error check for unmounting drives ...

@adriaandegroot

This comment has been minimized.

Contributor

adriaandegroot commented Nov 1, 2018

This is probably relevant too:

src/modules/unpackfs/main.py:            shutil.rmtree(source_mount_path, ignore_errors=True, onerror=None)

For do-not-format partitions, the whole cleanup process should be avoided entirely.

@philmmanjaro

This comment has been minimized.

Member

philmmanjaro commented Nov 1, 2018

@adriaandegroot: it happens already in the disk-probing during displaying the welcome screen. The user doesn't have to interact with CAL at all. unpackfs is a different story.

@adriaandegroot

This comment has been minimized.

Contributor

adriaandegroot commented Nov 2, 2018

In master I've followed @philmmanjaro suggestion, with some extra bits and pieces.

@aliveafter1000

This comment has been minimized.

aliveafter1000 commented Nov 2, 2018

You wrote mountsDir.setAutoRemove( false ); twice.
Should only turn it off when mount succeeds and unmount fails.
Also mount as read only. I don't see a need for read-write mounting.
People's data can be destroyed in a variety of random ways besides QTemporaryDir destructor.

static FstabEntryList
lookForFstabEntries( const QString& partitionPath )
{
    FstabEntryList fstabEntries;
    QTemporaryDir mountsDir;
    mountsDir.setAutoRemove( false );

    int exit = QProcess::execute( "mount", { partitionPath, mountsDir.path() } );
    if ( !exit ) // if all is well
    {
        QFile fstabFile( mountsDir.path() + "/etc/fstab" );
        if ( fstabFile.open( QIODevice::ReadOnly | QIODevice::Text ) )
        {
            const QStringList fstabLines = QString::fromLocal8Bit( fstabFile.readAll() )
                                     .split( '\n' );

            for ( const QString& rawLine : fstabLines )
                fstabEntries.append( FstabEntry::fromEtcFstab( rawLine ) );
            fstabFile.close();
            std::remove_if( fstabEntries.begin(), fstabEntries.end(), [](const FstabEntry& x) { return !x.isValid(); } );
        }

        if ( QProcess::execute( "umount", { "-R", mountsDir.path() } ) )
        {
            cWarning() << "Could not unmount" << mountsDir.path();
            // There is stuff left in there, really don't remove
            mountsDir.setAutoRemove( false );
        }
    }

    return fstabEntries;
}
@philmmanjaro

This comment has been minimized.

Member

philmmanjaro commented Nov 2, 2018

@adriaandegroot: shouldn't we go like this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment