Don't overwrite fstab, even when root mount is missing

Bug #1955019 reported by Łukasz Zemczak
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ubuntu Image
Fix Released
Medium
Paul Mars

Bug Description

Bug reported by Dave Jones (waveform) via github, copy-pasting it here into Launchpad:

As a follow-on to #30, while testing the new ubuntu-image with some proposed livecd-rootfs changes I found another incorrect corner case: if the fstab has no root partition, but contains other entries (e.g. a swap partition, or a boot partition), the entire fstab is clobbered and re-written with one just containing the LABEL=writable root partition.

Admittedly, this should never occur in practice because livecd-rootfs should always produce an fstab with a root mount-point (as indeed it does at the moment), but the logic is still clearly wrong.

The current logic is as follows:

ubuntu-image/internal/statemachine/classic_states.go

Lines 186 to 196 in 95689d1
 if !strings.Contains(string(fstabBytes), "LABEL=writable") {
  re := regexp.MustCompile(`(?m:^LABEL=\S+\s+/\s+(.*)$)`)
  newContents := re.ReplaceAll(fstabBytes, []byte("LABEL=writable\t/\t$1"))
  if !strings.Contains(string(newContents), "LABEL=writable") {
   newContents = []byte("LABEL=writable / ext4 defaults 0 0")
  }
  err := ioutilWriteFile(fstabPath, newContents, 0644)
  if err != nil {
   return fmt.Errorf("Error writing to fstab: %s", err.Error())
  }
 }

I would suggest this should be changed to:

    Always parse the fstab regardless of whether LABEL=writable is present or not (even if it is present, there's no guarantee it's not just in a comment)
    Don't assume any existing root mount-point uses a LABEL= scheme (as the current regex does)
    Preferably don't use a regex to find the root mount-point. The format is easy enough to parse "properly" by simple whitespace-splitting at which point a trivial field[1] == '/' test should be sufficient
    Append the root mount-point definition if none are found but leave the rest of the file intact
    Eliminate subsequent root mount-point definitions if multiple are found (or would it be preferable to crash'n'burn? It might be...)
    Use the correct default mount options and fsck flag
    Ensure the file is produced with a newline at the end (to facilitate further script-based processing; the current code outputs a file with no ending NL in the case the file is re-written)

The following awk code implements the above suggestions and could serve as a guide for what needs to be implemented in ubuntu-image (cribbed from my proposed merge for livecd-rootfs). It assumes that root_fs_label and root_fs_options are externally defined (which they are in the script to "cloudimg-rootfs" and "discard,errors=remount-ro" respectively)

# Use tab separators on any modified lines
BEGIN { OFS="\t"; count=0; }

# Omit the "UNCONFIGURED" warning if it is still present
/^# UNCONFIGURED FSTAB/ { next; }

# Only modify the first non-comment line where the second field is
# the root and omit multiple root definitions
/^[^#]/ && $2 == "/" {
    if (!count) {
        $1="LABEL=" root_fs_label;
        $4=root_fs_options;
        $6="1";
    }
    count++;
    if (count > 1) next;
}

# Output the (potentially modified) line
{ print; }

# If we reach the end without seeing a root mount line, add one
END {
    if (!count) {
        print "LABEL=" root_fs_label, "/", "ext4", root_fs_options, "0", "1";
    }
}

Changed in ubuntu-image:
importance: Undecided → Medium
Paul Mars (upils)
Changed in ubuntu-image:
status: New → In Progress
assignee: nobody → Paul Mars (upils)
Paul Mars (upils)
Changed in ubuntu-image:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.