[gparted] Remove coding landmine in get_disk() (#152)
- From: Curtis Gedak <gedakc src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [gparted] Remove coding landmine in get_disk() (#152)
- Date: Thu, 15 Apr 2021 16:57:17 +0000 (UTC)
commit c4ef43aa5d551569ca5cad5912d51f0f9df261d2
Author: Mike Fleetwood <mike fleetwood googlemail com>
Date: Sat Apr 10 16:39:22 2021 +0100
Remove coding landmine in get_disk() (#152)
get_disk() is the wrapper around libparted's ped_disk_new() which reads
a disk label from the specified device and if successful creates the in
memory PedDisk object to represent it. In the case that libparted
doesn't recognise a disk label or a file system, having get_disk() go
and destroy the passed in PedDevice object via parameter lp_device is
very unexpected behaviour hence describing it as a coding landmine.
BACKGROUND
1. Early on GParted only worked with devices with valid disk labels.
FileSystem.h:open_device_and_disk() required both ped_device_get()
and ped_disk_new() to succeed or neither to succeed.
2. Commit [1] added support for devices which didn't yet have a disk
label. open_device_and_disk() had default parameter strict=true
added. While scanning strict=false was passed which allowed
open_device_and_disk() to return success if only ped_device_get()
succeeded and ped_disk_new() failed when the disk was empty. All
other times open_device_and_disk() was called with default
strict=true, still requiring both or neither to succeed.
3. Commit [2] added support for whole disk file systems. The now named
get_device_and_disk() had it's functionality split between
get_device() and get_disk(). This result in the code landmine being
left behind: get_disk() destroying the passed device object if
default parameter strict=true and no disk label or file system was
detected.
ANALYSIS
1. Since support for whole disk file systems [2] all current calls to
get_device_and_disk() let the strict parameter default to true and
are only called on known partitions within disk labels when applying
a change to that partition. Therefore they don't care about the
behaviour of get_disk(), just that get_device_and_disk() maintains
that both ped_device_get() and ped_disk_new() succeed or neither
succeed.
2. Two direct calls to get_disk() where the strict parameter defaults to
true, from calibrate_partition() and erase_filesystem_signatures(),
only do so on known partitions within disk labels as part of applying
a change to that partition. Therefore ped_disk_new() will succeed
and so PedDevice isn't deleted when not wanted.
3. The two remaining direct calls to get_disk() where the strict
parameter is explicitly set to false, from set_device_from_disk() and
detect_filesystem_in_encryption_mapping(), are when scanning. As the
pass strict=false they don't allow the PedDevice deletion to occur if
no recognised disk label is found.
FIX
Remove the strict parameter from get_disk() and get_device_and_disk() as
it's no longer needed. Remove the code landmine by removing the side
affect of destroying the PedDevice object if a disk label isn't found.
Make sure get_device_and_disk() maintains the all or nothing behaviour.
Also don't pass lp_device by reference to a pointer to get_disk() so the
code can't change where lp_device points.
[1] 038c5c5d99ad842f1a57f12222c884be29f4df4f
P (special thanks to mantiena-baltix for bringing this issue to my
[2] 51ac4d56489653854cd22787238a14bfa66a6ad4
Split get_device_and_disk() into two (#743181)
Closes #152 - GParted crashed when trying to probe an encrypted
partition containing content that libparted doesn't
recognise
include/GParted_Core.h | 6 +++---
src/GParted_Core.cc | 43 ++++++++++++++++++++-----------------------
2 files changed, 23 insertions(+), 26 deletions(-)
---
diff --git a/include/GParted_Core.h b/include/GParted_Core.h
index 0db4aa73..904b0923 100644
--- a/include/GParted_Core.h
+++ b/include/GParted_Core.h
@@ -222,9 +222,9 @@ private:
static bool flush_device( PedDevice * lp_device );
static bool get_device( const Glib::ustring & device_path, PedDevice *& lp_device, bool flush = false
);
- static bool get_disk( PedDevice *& lp_device, PedDisk *& lp_disk, bool strict = true );
- static bool get_device_and_disk( const Glib::ustring & device_path,
- PedDevice*& lp_device, PedDisk*& lp_disk, bool strict = true, bool
flush = false );
+ static bool get_disk(PedDevice *lp_device, PedDisk*& lp_disk);
+ static bool get_device_and_disk(const Glib::ustring& device_path,
+ PedDevice*& lp_device, PedDisk*& lp_disk, bool flush = false);
static void destroy_device_and_disk( PedDevice*& lp_device, PedDisk*& lp_disk );
static bool commit( PedDisk* lp_disk );
static bool commit_to_os( PedDisk* lp_disk, std::time_t timeout );
diff --git a/src/GParted_Core.cc b/src/GParted_Core.cc
index 6d6885ac..b6a45644 100644
--- a/src/GParted_Core.cc
+++ b/src/GParted_Core.cc
@@ -687,7 +687,7 @@ void GParted_Core::set_device_from_disk( Device & device, const Glib::ustring &
set_device_one_partition( device, lp_device, fstype, messages );
}
// Partitioned drive
- else if ( get_disk( lp_device, lp_disk, false ) )
+ else if (get_disk(lp_device, lp_disk))
{
// Partitioned drive (excluding "loop"), as recognised by libparted
if ( lp_disk && lp_disk->type && lp_disk->type->name &&
@@ -1089,7 +1089,7 @@ FSType GParted_Core::detect_filesystem_in_encryption_mapping(const Glib::ustring
// supports one block device to one encryption mapping to one file system.
PedDisk *lp_disk = NULL;
PedPartition *lp_partition = NULL;
- if (get_disk(lp_device, lp_disk, false) && lp_disk && lp_disk->type &&
+ if (get_disk(lp_device, lp_disk) && lp_disk && lp_disk->type &&
lp_disk->type->name && strcmp(lp_disk->type->name, "loop") == 0 )
{
lp_partition = ped_disk_next_partition(lp_disk, NULL);
@@ -4104,41 +4104,38 @@ bool GParted_Core::get_device( const Glib::ustring & device_path, PedDevice *& l
return false;
}
-bool GParted_Core::get_disk( PedDevice *& lp_device, PedDisk *& lp_disk, bool strict )
+
+bool GParted_Core::get_disk(PedDevice *lp_device, PedDisk*& lp_disk)
{
- if ( lp_device )
- {
- lp_disk = ped_disk_new( lp_device );
+ g_assert(lp_device != NULL); // Bug: Not initialised by call to ped_device_get() or
ped_device_get_next()
- // (#762941)(!46) After ped_disk_new() wait for triggered udev rules to
- // to complete which remove and re-add all the partition specific /dev
- // entries to avoid FS specific commands failing because they happen to
- // be running when the needed /dev/PTN entries don't exist.
- settle_device(SETTLE_DEVICE_PROBE_MAX_WAIT_SECONDS);
+ lp_disk = ped_disk_new(lp_device);
- // if ! disk and writable it's probably a HD without disklabel.
- // We return true here and deal with them in
- // GParted_Core::set_device_from_disk().
- if ( lp_disk || ( ! strict && ! lp_device ->read_only ) )
- return true;
+ // (#762941)(!46) After ped_disk_new() wait for triggered udev rules to complete
+ // which remove and re-add all the partition specific /dev entries to avoid FS
+ // specific commands failing because they happen to be running when the needed
+ // /dev/PTN entries don't exist.
+ settle_device(SETTLE_DEVICE_PROBE_MAX_WAIT_SECONDS);
- destroy_device_and_disk( lp_device, lp_disk );
- }
-
- return false;
+ return (lp_disk != NULL);
}
-bool GParted_Core::get_device_and_disk( const Glib::ustring & device_path,
- PedDevice*& lp_device, PedDisk*& lp_disk, bool strict, bool flush )
+
+bool GParted_Core::get_device_and_disk(const Glib::ustring& device_path,
+ PedDevice*& lp_device, PedDisk*& lp_disk, bool flush)
{
if ( get_device( device_path, lp_device, flush ) )
{
- return get_disk( lp_device, lp_disk, strict );
+ if (get_disk(lp_device, lp_disk))
+ return true;
+
+ destroy_device_and_disk(lp_device, lp_disk);
}
return false;
}
+
void GParted_Core::destroy_device_and_disk( PedDevice*& lp_device, PedDisk*& lp_disk )
{
if ( lp_disk )
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]