growpart mishandles image filenames that end in a number

Bug #1835124 reported by David Krauser on 2019-07-02
18
This bug affects 1 person
Affects Status Importance Assigned to Milestone
cloud-utils
High
Unassigned
cloud-utils (Ubuntu)
Low
Scott Moser
Disco
Low
Rafael David Tinoco

Bug Description

[Impact]

  * DEP8 race condition for ppc64el (LP: #1836593): intermittent migration regressions.

  * growpart: fix bug when file image ends in a digit (LP: #1835124): image files can't end in ".ext4", for example, orelse growpart doesn't work.

  * fix spelling error in ec2metadata (LP: #1810857): no impact.

[Test Case]

(k)inaddy@kvirtclone:~$ sudo fdisk /fakedisk.ext4

(m for help): d
Selected partition 1
Partition 1 has been deleted.

Command (m for help): n
Partition type
   p primary (0 primary, 0 extended, 4 free)
   e extended (container for logical partitions)
Select (default p): p
Partition number (1-4, default 1):
First sector (2048-262143, default 2048):
Last sector, +/-sectors or +/-size{K,M,G,T,P} (2048-262143, default 262143): +100mb

Created a new partition 1 of type 'Linux' and of size 95 MiB.

Command (m for help): w
The partition table has been altered.
Syncing disks.

(k)inaddy@kvirtclone:~$ sudo growpart /fakedisk.ext4 1
FAILED: failed to get start and end for /fakedisk.ext41 in /fakedisk.ext4

[Regression Potential]

 * Fix tries to recognize if volume is a block device or not. Same code exists for block devices, which reduces probability of issues.
 * Fix was done by pkg maintainer and it is already upstreamed.

[Other Info]
 * Related Bugs:
   * bug 1842682: regression in test-growpart results in test fail device or resource busy

 * Original description:

When growpart attempts to determine the partition to resize, it uses this logic:

$ sed -n '266,275p' $(which growpart)
        dpart="${DISK}${PART}" # disk and partition number
        if [ -b "${DISK}p${PART}" -a "${DISK%[0-9]}" != "${DISK}" ]; then
                # for block devices that end in a number (/dev/nbd0)
                # the partition is "<name>p<partition_number>" (/dev/nbd0p1)
                dpart="${DISK}p${PART}"
        elif [ "${DISK#/dev/loop[0-9]}" != "${DISK}" ]; then
                # for /dev/loop devices, sfdisk output will be <name>p<number>
                # format also, even though there is not a device there.
                dpart="${DISK}p${PART}"
        fi

If the disk is an image, and the image filename ends with a number, the partition will be "${DISK}p${PART}"; however, "${DISK}p${PART}" will not be a block device. Thus, the partition is improperly identified as just "${DISK}${PART}".

This gives us a failure like:

+ growpart -v -v -v disk-uefi.ext4 1
update-partition set to true
resizing 1 on disk-uefi.ext4 using resize_sfdisk_gpt
running[sfd_list][erronly] sfdisk --list --unit=S disk-uefi.ext4
6291456 sectors of 512. total size=3221225472 bytes
running[sfd_dump][erronly] sfdisk --unit=S --dump disk-uefi.ext4
## sfdisk --unit=S --dump disk-uefi.ext4
label: gpt
label-id: A9F73A73-50FD-4335-9082-1249985F154D
device: disk-uefi.ext4
unit: sectors
first-lba: 34
last-lba: 6291422

disk-uefi.ext4p1 : start= 227328, size= 4384735, type=0FC63DAF-8483-4772-8E79-3D69D8477DE4, uuid=C1191CD2-0753-4A53-8CD4-E6079735CA42
disk-uefi.ext4p14 : start= 2048, size= 8192, type=21686148-6449-6E6F-744E-656564454649, uuid=3A2AD377-EB6D-4689-9126-35148C003A95
disk-uefi.ext4p15 : start= 10240, size= 217088, type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B, uuid=98C675C8-4FF6-425C-B783-E77FDE70C967
FAILED: failed to get start and end for disk-uefi.ext41 in disk-uefi.ext4

Related branches

Joshua Powers (powersj) on 2019-07-08
Changed in cloud-utils:
status: New → Triaged
importance: Undecided → High
tags: added: server-next
Changed in cloud-utils (Ubuntu):
status: New → Confirmed
status: Confirmed → Triaged
Changed in cloud-utils:
status: Triaged → Confirmed

cloud-utils growpart script uses either sgdisk or sfdisk as a resizer... unfortunately it does not expect disk formats other than:

resize_sfdisk_gpt() {
 resize_sfdisk gpt
}

resize_sfdisk_dos() {
 resize_sfdisk dos
}

where $1 is the type of the disk format.

that is why you're getting this error. I'll flag this as "Triaged" and "Wishlist", and put in server team queue for further analysis.

Thanks for reporting this!

Changed in cloud-utils (Ubuntu):
importance: Undecided → Wishlist
David Krauser (davidkrauser) wrote :

The error occurs with a gpt formatted image, which is supported by sfdisk.

This logic in growpart is broken for image files:

if [ -b "${DISK}p${PART}" -a "${DISK%[0-9]}" != "${DISK}" ]; then

If an image filename ends with a number, sfdisk identifies each partition like:

${DISK}p${PART_NUMBER}

Each partition is _not_ a block device, though, so they fail the `-b "${DISK}p${PART}"` check.

Removing that specific check fixes this issue; however, I don't know what ramification that has for other use cases.

As a workaround, we are renaming the image file to end with a non-numerical suffix, growing the partition, then renaming the image back to its original name.

I see what you mean now David, sorry for the wrong assumption, let me re-check this for you.

Scott Moser (smoser) wrote :

I agree that this is a bug (fix proposed), but fwiw, your naming of a *disk* image with an extension 'ext4' is a bit confusing to anyone that might see that file. Probably better '.disk' or '.img'.

Yes Scott, I do agree, and that is why I thought this was related to a loopback filesystem only, having a GPT/MSDOS partition table in a file called .ext4 is awkward to me as well. Thank you very much for your path, I'm reviewing it right now.

Download full text (3.8 KiB)

(k)inaddy@kvirtclone:~$ sudo fdisk /fakedisk

Welcome to fdisk (util-linux 2.33.1).
Changes will remain in memory only, until you decide to write them.
Be careful before using the write command.

Command (m for help): p
Disk /fakedisk: 128 MiB, 134217728 bytes, 262144 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: dos
Disk identifier: 0x9953d6a9

Device Boot Start End Sectors Size Id Type
/fakedisk1 2048 262143 260096 127M 83 Linux

Command (m for help): d
Selected partition 1
Partition 1 has been deleted.

Command (m for help): n
Partition type
   p primary (0 primary, 0 extended, 4 free)
   e extended (container for logical partitions)
Select (default p):

Using default response p.
Partition number (1-4, default 1):
First sector (2048-262143, default 2048):
Last sector, +/-sectors or +/-size{K,M,G,T,P} (2048-262143, default 262143): +100mb

Created a new partition 1 of type 'Linux' and of size 95 MiB.

(k)inaddy@kvirtclone:~$ sudo qemu-nbd -c /dev/nbd8 -f raw /fakedisk

(k)inaddy@kvirtclone:~$ ls /dev/nbd8*
/dev/nbd8 /dev/nbd8p1

(k)inaddy@kvirtclone:~$ sudo growpart /dev/nbd8 1
CHANGED: partition=1 start=2048 old: size=194560 end=196608 new: size=260063 end=262111

(k)inaddy@kvirtclone:~$ sudo mv /fakedisk /fakedisk.ext4

(k)inaddy@kvirtclone:~$ sudo fdisk /fakedisk.ext4

(m for help): d
Selected partition 1
Partition 1 has been deleted.

Command (m for help): n
Partition type
   p primary (0 primary, 0 extended, 4 free)
   e extended (container for logical partitions)
Select (default p): p
Partition number (1-4, default 1):
First sector (2048-262143, default 2048):
Last sector, +/-sectors or +/-size{K,M,G,T,P} (2048-262143, default 262143): +100mb

Created a new partition 1 of type 'Linux' and of size 95 MiB.

Command (m for help): p
Disk /fakedisk.ext4: 128 MiB, 134217728 bytes, 262144 sectors
Units: sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disklabel type: dos
Disk identifier: 0x9953d6a9

Device Boot Start End Sectors Size Id Type
/fakedisk.ext4p1 2048 196607 194560 95M 83 Linux

Command (m for help): w
The partition table has been altered.
Syncing disks.

(k)inaddy@kvirtclone:~$ sudo growpart /fakedisk.ext4 1
FAILED: failed to get start and end for /fakedisk.ext41 in /fakedisk.ext4

### BEFORE YOUR PATCH ^^^^^^^^

### AFTER YOUR PATCH vvvvvvvvv

(k)inaddy@kvirtclone:~/work/sources/ubuntu/cloud-utils$ sudo dpkg -i ./*.deb
(Reading database ... 129124 files and directories currently installed.)
Preparing to unpack .../cloud-guest-utils_0.30-52-g97fddc7b-0ubuntu1_all.deb ...
Unpacking cloud-guest-utils (0.30-52-g97fddc7b-0ubuntu1) over (0.30-51-g7adb670f-0ubuntu1) ...
Preparing to unpack .../cloud-image-utils_0.30-52-g97fddc7b-0ubuntu1_all.deb ...
Unpacking cloud-image-utils (0.30-52-g97fddc7b-0ubuntu1) over (0.30-51-g7adb670f-0ubuntu1) ...
Preparing to unpack .../cloud-utils-euca_0.30-52-g97fddc7b-0ubuntu1_all.deb ...
Unpacking cloud-utils-euca (0.30-52-g97fddc7b-0ubuntu1) over (0...

Read more...

Changed in cloud-utils (Ubuntu):
importance: Wishlist → Low
status: Triaged → In Progress
assignee: nobody → Scott Moser (smoser)

Scott shouldn't this be first merge into upstream first ? Then I can merge eoan with upstream and SRU the fix for other versions as DEP3.

Scott Moser (smoser) on 2019-07-10
Changed in cloud-utils:
status: Confirmed → Fix Committed
Changed in cloud-utils (Ubuntu):
assignee: Scott Moser (smoser) → nobody
Scott Moser (smoser) wrote :

@Rafael, yes. Fixed upstream now.

I'm going to also do an upload to eoan using 'new-upstream-snapshot'
[1] https://github.com/CanonicalLtd/uss-tableflip/blob/master/scripts/new-upstream-snapshot

In order to make that workflow work, i did have to get tags into git from the old bzr repo and sign those tags.

Changed in cloud-utils (Ubuntu):
assignee: nobody → Scott Moser (smoser)

Thanks a lot!

David Krauser (davidkrauser) wrote :

Thank you both for addressing the issue :-)

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package cloud-utils - 0.31-3-gfadd07fe-0ubuntu1

---------------
cloud-utils (0.31-3-gfadd07fe-0ubuntu1) eoan; urgency=medium

  * New upstream snapshot.
    - growpart: Fix bug when file image ends in a digit. (LP: #1835124)
    - Fix spelling error in ec2metadata (--reserveration-id).
      (LP: #1810857)
    - tools: rename export-tarball to make-tarball.

 -- Scott Moser <email address hidden> Wed, 10 Jul 2019 14:09:37 -0400

Changed in cloud-utils (Ubuntu):
status: In Progress → Fix Released
Changed in cloud-utils (Ubuntu Disco):
status: New → In Progress
assignee: nobody → Rafael David Tinoco (rafaeldtinoco)
importance: Undecided → Low
Robie Basak (racb) wrote :

SRU information required in this bug please.

Sorry Robie,

I missed the template. Now its good to go I believe.

description: updated

Hello David, or anyone else affected,

Accepted cloud-utils into disco-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/cloud-utils/0.31-0ubuntu1.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-disco to verification-done-disco. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-disco. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in cloud-utils (Ubuntu Disco):
status: In Progress → Fix Committed
tags: added: verification-needed verification-needed-disco

All autopkgtests for the newly accepted cloud-utils (0.31-0ubuntu1.1) for disco have finished running.
The following regressions have been reported in tests triggered by the package:

cloud-utils/0.31-0ubuntu1.1 (amd64, i386, ppc64el, arm64, s390x)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/disco/update_excuses.html#cloud-utils

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

David Krauser (davidkrauser) wrote :

We ran into this bug initially on xenial when doing image builds. Manually installing the cloud-guest-utils 0.31-0ubuntu1.1 deb in that environment resolves the issue.

Scott Moser (smoser) wrote :

The excuses page linked in comment 15 lists a test failure that is a regression of the change for this bug.

I've filed bug 1842682 and put up a pull request to fix that logic error.

description: updated

Alright, I'll backport LP: #1842682 SRU together with this one and propose it in the existing merge for review.

MR:

https://code.launchpad.net/~rafaeldtinoco/ubuntu/+source/cloud-utils/+git/cloud-utils/+ref/lp1835124

rebases previous merge including your fix.

Feel free to sponsor if review is +1.

Let me know if there are any issues.

Thx Scott!

For whoever interests, the version fixing the issue with autopkgtests (that blocked migration of 0.31-0ubuntu1.1) is 0.31-0ubuntu1.2. So there will be a new fix to be verified in this bug.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers