Bug#894549: debhelper: dh_usrlocal may remove a direct subdirectory of /usr/local

[Niels Thykier]

On Fri, 6 Apr 2018 21:37:39 +0200 Nicolas Boulenguez
 wrote:
> > use the "hashref" based autoscript?
> 
> The attached updated diff switches
> from strings and sed expressions
> back to arrays (again) and hashrefs (new).
> 

Ok.  Thanks for doing the hashref rewrite. :)

> > The "justdirs" is supposed to be in "opposite" order of "dirs"
> > ("justdirs" are in "removal order" while "dirs" are in "creation order")
> > but as far as I can see that ordering is not preserved in the patch.
> 
> They are not supposed to be in opposite order.

True.

> [...]
> 
> * @justdirs is appended all childs before their parent.  The list is
>   constructed in the same code part actually removing the directory
>   from $tmpdir/usr/local/, so this order is compatible with
>   uninstallation
> 

This was the bit I was missing.  thanks. :)

> [...]
> 
> > I think dh_usrlocal might be ready for some test cases to avoid future
> > breakage.
> 
> The attached test.sh implements some suggested tests.

I have adapted them and included them t/dh_usrlocal/01-basic.t.

Thanks for your work - I am very happy that you helped me improve
dh_usrlocal and include some tests for it as well to avoid future
regressions.  :)

Thanks,
~Niels

Bug#894549: debhelper: dh_usrlocal may remove a direct subdirectory of /usr/local

[Nicolas Boulenguez]

> use the "hashref" based autoscript?

The attached updated diff switches
from strings and sed expressions
back to arrays (again) and hashrefs (new).

> The "justdirs" is supposed to be in "opposite" order of "dirs"
> ("justdirs" are in "removal order" while "dirs" are in "creation order")
> but as far as I can see that ordering is not preserved in the patch.

They are not supposed to be in opposite order.
Actually, @justdirs contains strictly less elements than @dirs.
During the same recursive traversal,

* @dirs is appended each parent before its childs. This is the order
  in which subdirectories are discovered in $tmpdir/usr/local/ during
  the traversal, so it is by construction compatible with later
  re-creation during install.

* @justdirs is appended all childs before their parent.  The list is
  constructed in the same code part actually removing the directory
  from $tmpdir/usr/local/, so this order is compatible with
  uninstallation

* In both lists, the childs are traversed in alphabetical order to
  ensure reproducible scripts. This is unrelated with the two previous
  constraints.

If this explanation seems convincing, please paste it inside comments
before the File::Find::file recursion.

> I think dh_usrlocal might be ready for some test cases to avoid future
> breakage.

The attached test.sh implements some suggested tests.
--- a/dh_usrlocal
+++ b/dh_usrlocal
@@ -77,54 +77,49 @@
 
 	if (-d "$tmp/usr/local") {
 		my (@dirs, @justdirs);
-		find({bydepth => 1,
-		  no_chdir => 1,
+		find({no_chdir => 1,
+		  preprocess => sub {
+			# Ensure a reproducible traversal.
+			return sort @_;
+		  },
+		  postprocess => sub {
+			# Uninstall, unless a direct child of /usr/local.
+			$_ = $File::Find::dir;
+			s!^\Q$tmp\E!!;
+			push @justdirs, $_ if m!/usr/local/.*/!;
+			# Remove a directory after its childs.
+			doit('rmdir', $File::Find::dir);
+		  },
 		  wanted => sub {
+			# rmdir would fail later anyways.
+			error("${File::Find::name} is not a directory")
+if not -d $File::Find::name;
+			# Install directory before its childs.
 			my $fn = $File::Find::name;
-			if (-d $fn) {
-my $user = 'root';
-my $group = 'staff';
-my $mode = '02775';
-if (should_use_root()) {
-	my $stat = stat $fn;
-	$user = getpwuid $stat->uid;
-	$group = getgrgid $stat->gid;
-	$mode = sprintf "%04lo", ($stat->mode & 0);
-	if ($stat->uid == 0 && $stat->gid == 0) {
-		$group = 'staff';
-		$mode = '02775';
-	}
+			$fn =~ s!^\Q$tmp\E!!;
+			return if $fn eq '/usr/local';
+			if (should_use_root()) {
+my $stat = stat $File::Find::dir;
+if ($stat->uid == 0 && $stat->gid == 0) {
+	push @dirs, "$fn 02775 root staff";
+} else {
+	my $user = getpwuid $stat->uid;
+	my $group = getgrgid $stat->gid;
+	my $mode = sprintf "%04lo", ($stat->mode & 0);
+	push @dirs, "$fn $mode $user $group";
 }
-
-
-
-$fn =~ s!^\Q$tmp\E!!;
-return if $fn eq '/usr/local';
-
-# @dirs is in parents-first order for dir creation...
-unshift @dirs, "$fn $mode $user $group";
-# ...whereas @justdirs is depth-first for removal.
-push @justdirs, $fn;
-doit('rmdir', $_);
-			}
-			else {
-warning("$fn is not a directory");
+			} else {
+push @dirs, "$fn 02775 root staff";
 			}
 		  }}, "$tmp/usr/local");
-		doit('rmdir', "$tmp/usr/local");
-	
-		my $bs = "\\"; # A single plain backslash
-		my $ebs = $bs x 2; # Escape the backslash from the shell
+
 		# This constructs the body of a 'sed' c\ expression which
 		# is parsed by the shell in double-quotes
-		my $dirs = join("$ebs\n", sort @dirs);
-		pop @justdirs; # don't remove directories directly in /usr/local
-		my $justdirs = join("$ebs\n", reverse sort @justdirs);
 		if (! $dh{NOSCRIPTS}) { 
 			autoscript($package,"postinst", "postinst-usrlocal",
-   "/#DIRS#/ c${ebs}\n${dirs}");
+   { 'DIRS' => join ("\n", @dirs)}) if @dirs;
 			autoscript($package,"prerm", "prerm-usrlocal",
-   "/#JUSTDIRS#/ c${ebs}\n${justdirs}") if length $justdirs;
+   { 'JUSTDIRS' => join ("\n", @justdirs)}) if @justdirs;
 		}
 	}
 }


test.sh
Description: Bourne shell script

Bug#894549: debhelper: dh_usrlocal may remove a direct subdirectory of /usr/local

[Niels Thykier]

On Wed, 04 Apr 2018 18:29:00 + Niels Thykier  wrote:
> On Mon, 2 Apr 2018 16:53:52 +0200 Nicolas Boulenguez
>  wrote:
> > Package: debhelper
> > Followup-For: Bug #894549
> > 
> > Hello.
> > 
> > This new diff should fix the same issues in dh_usrlocal.
> > The result is more readable in my opinion, hopefully preventing
> > similar errors in the future.
> > 
> > It should also be a bit more efficient: instead of sorting two huge
> > lists of paths all starting with "/usr/share/", it sorts once simple
> > names in each directory.
> > 
> > When a normal file is encountered, it displays an error instead of a
> > warning. This does not change much, as rmdir would fail with a less
> > explicit message when attempting to remove the containing directory.
> 
> Hi Nicolas,
> 
> Thanks for your finding and the patch.
> 
> While we are at the subject of making it more readable, perhaps I could
> ask you convert dh_usrlocal to use the "hashref" based autoscript?  That
> should avoid the need for the "\\\"-mess we currently have.
> 
> Also, many thanks for the other bugs you have filed recently.  I am very
> grateful that you have included patches in them. :)
> 
> Thanks,
> ~Niels
> 
> 

Just double checked this one:  I think there is a bug in your proposed
patch.  The "justdirs" is supposed to be in "opposite" order of "dirs"
("justdirs" are in "removal order" while "dirs" are in "creation order")
but as far as I can see that ordering is not preserved in the patch.

I think dh_usrlocal might be ready for some test cases to avoid future
breakage.  If you want to have a look at that, it would be appreciated
as well - otherwise, I will probably look at it when I review your
updated patch.

Thanks,
~Niels

Bug#894549: debhelper: dh_usrlocal may remove a direct subdirectory of /usr/local

[Niels Thykier]

On Mon, 2 Apr 2018 16:53:52 +0200 Nicolas Boulenguez
 wrote:
> Package: debhelper
> Followup-For: Bug #894549
> 
> Hello.
> 
> This new diff should fix the same issues in dh_usrlocal.
> The result is more readable in my opinion, hopefully preventing
> similar errors in the future.
> 
> It should also be a bit more efficient: instead of sorting two huge
> lists of paths all starting with "/usr/share/", it sorts once simple
> names in each directory.
> 
> When a normal file is encountered, it displays an error instead of a
> warning. This does not change much, as rmdir would fail with a less
> explicit message when attempting to remove the containing directory.

Hi Nicolas,

Thanks for your finding and the patch.

While we are at the subject of making it more readable, perhaps I could
ask you convert dh_usrlocal to use the "hashref" based autoscript?  That
should avoid the need for the "\\\"-mess we currently have.

Also, many thanks for the other bugs you have filed recently.  I am very
grateful that you have included patches in them. :)

Thanks,
~Niels

Bug#894549: debhelper: dh_usrlocal may remove a direct subdirectory of /usr/local

[Nicolas Boulenguez]

Package: debhelper
Followup-For: Bug #894549

Hello.

This new diff should fix the same issues in dh_usrlocal.
The result is more readable in my opinion, hopefully preventing
similar errors in the future.

It should also be a bit more efficient: instead of sorting two huge
lists of paths all starting with "/usr/share/", it sorts once simple
names in each directory.

When a normal file is encountered, it displays an error instead of a
warning. This does not change much, as rmdir would fail with a less
explicit message when attempting to remove the containing directory.
--- a/dh_usrlocal
+++ b/dh_usrlocal
@@ -76,55 +76,50 @@
 	my $tmp = tmpdir($package);
 
 	if (-d "$tmp/usr/local") {
-		my (@dirs, @justdirs);
-		find({bydepth => 1,
-		  no_chdir => 1,
+		my ($dirs, $justdirs);
+		find({no_chdir => 1,
+		  preprocess => sub {
+			# Ensure a reproducible traversal.
+			return sort @_;
+		  },
+		  postprocess => sub {
+			# Uninstall, unless a direct child of /usr/local.
+			$_ = $File::Find::dir;
+			s!^\Q$tmp\E!!;
+			$justdirs .= "\n$_" if m!/usr/local/.*/!;
+			# Remove a directory after its childs.
+			doit('rmdir', $File::Find::dir);
+		  },
 		  wanted => sub {
+			# rmdir would fail later anyways.
+			error("${File::Find::name} is not a directory")
+if not -d $File::Find::name;
+			# Install directory before its childs.
 			my $fn = $File::Find::name;
-			if (-d $fn) {
-my $user = 'root';
-my $group = 'staff';
-my $mode = '02775';
-if (should_use_root()) {
-	my $stat = stat $fn;
-	$user = getpwuid $stat->uid;
-	$group = getgrgid $stat->gid;
-	$mode = sprintf "%04lo", ($stat->mode & 0);
-	if ($stat->uid == 0 && $stat->gid == 0) {
-		$group = 'staff';
-		$mode = '02775';
-	}
+			$fn =~ s!^\Q$tmp\E!!;
+			return if $fn eq '/usr/local';
+			if (should_use_root()) {
+my $stat = stat $File::Find::dir;
+if ($stat->uid == 0 && $stat->gid == 0) {
+	$dirs .= "\n$fn 02775 root staff";
+} else {
+	my $user = getpwuid $stat->uid;
+	my $group = getgrgid $stat->gid;
+	my $mode = sprintf "%04lo", ($stat->mode & 0);
+	$dirs .= "\n$fn $mode $user $group";
 }
-
-
-
-$fn =~ s!^\Q$tmp\E!!;
-return if $fn eq '/usr/local';
-
-# @dirs is in parents-first order for dir creation...
-unshift @dirs, "$fn $mode $user $group";
-# ...whereas @justdirs is depth-first for removal.
-push @justdirs, $fn;
-doit('rmdir', $_);
-			}
-			else {
-warning("$fn is not a directory");
+			} else {
+$dirs .= "\n$fn 02775 root staff";
 			}
 		  }}, "$tmp/usr/local");
-		doit('rmdir', "$tmp/usr/local");
-	
-		my $bs = "\\"; # A single plain backslash
-		my $ebs = $bs x 2; # Escape the backslash from the shell
+
 		# This constructs the body of a 'sed' c\ expression which
 		# is parsed by the shell in double-quotes
-		my $dirs = join("$ebs\n", sort @dirs);
-		pop @justdirs; # don't remove directories directly in /usr/local
-		my $justdirs = join("$ebs\n", reverse sort @justdirs);
 		if (! $dh{NOSCRIPTS}) { 
 			autoscript($package,"postinst", "postinst-usrlocal",
-   "/#DIRS#/ c${ebs}\n${dirs}");
+   "/#DIRS#/ c$dirs") if $dirs;
 			autoscript($package,"prerm", "prerm-usrlocal",
-   "/#JUSTDIRS#/ c${ebs}\n${justdirs}") if length $justdirs;
+   "/#JUSTDIRS#/ c$justdirs") if $justdirs;
 		}
 	}
 }

Bug#894549: debhelper: dh_usrlocal may remove a direct subdirectory of /usr/local

[Nicolas Boulenguez]

Package: debhelper
Version: 11.1.6
Severity: minor
Tags: patch

Hello.

The attached script shows that dh_usrlocal generates a prerm script
removing a direct subdirectory of /usr/local.
As I understand the policy, it should not.

The fix is included.
It also suggests some minor related changes, which do not seem to change the 
behaviour.
--- a/dh_usrlocal
+++ b/dh_usrlocal
@@ -82,6 +82,8 @@
 		  wanted => sub {
 			my $fn = $File::Find::name;
 			if (-d $fn) {
+return if $fn eq  "$tmp/usr/local";
+
 my $user = 'root';
 my $group = 'staff';
 my $mode = '02775';
@@ -99,12 +101,13 @@
 
 
 $fn =~ s!^\Q$tmp\E!!;
-return if $fn eq '/usr/local';
-
-# @dirs is in parents-first order for dir creation...
-unshift @dirs, "$fn $mode $user $group";
-# ...whereas @justdirs is depth-first for removal.
-push @justdirs, $fn;
+
+push @dirs, "$fn $mode $user $group";
+
+# don't remove directories directly in /usr/local
+if ($File::Find::dir ne "$tmp/usr/local") {
+	push @justdirs, $fn;
+}
 doit('rmdir', $_);
 			}
 			else {
@@ -118,11 +121,10 @@
 		# This constructs the body of a 'sed' c\ expression which
 		# is parsed by the shell in double-quotes
 		my $dirs = join("$ebs\n", sort @dirs);
-		pop @justdirs; # don't remove directories directly in /usr/local
 		my $justdirs = join("$ebs\n", reverse sort @justdirs);
 		if (! $dh{NOSCRIPTS}) { 
 			autoscript($package,"postinst", "postinst-usrlocal",
-   "/#DIRS#/ c${ebs}\n${dirs}");
+   "/#DIRS#/ c${ebs}\n${dirs}") if length $dirs;
 			autoscript($package,"prerm", "prerm-usrlocal",
    "/#JUSTDIRS#/ c${ebs}\n${justdirs}") if length $justdirs;
 		}


dh_usrlocal-topdirs.sh
Description: Bourne shell script