[pve-devel] [PATCH pve-network] dhcp: dnsmasq: Use dir_glob_foreach for deleting configuration files

[pve-devel] [PATCH pve-network] dhcp: dnsmasq: Use dir_glob_foreach for deleting configuration files

[Stefan Hanreich]

The current invocation is quite unsafe and triggers the taint mode of
Perl. Replacing it with dir_glob_foreach solves those issues.

Reported-By: Friedrich Weber <f.weber@proxmox.com>
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
I wasn't sure whether directly unlinking the files in the callback
would influence the iteration, hence why I store them in an
intermediate array. Also, unlinking them all at once probably is
better than unlinking them one-by-one (although it shouldn't matter
with the low amount of files here..)

 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index e65e973..2844943 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -234,7 +234,13 @@ CFG
 	$default_dnsmasq_config
     );
 
-    unlink glob "$config_directory/10-*.conf";
+    my @config_files = ();
+    PVE::Tools::dir_glob_foreach($config_directory, '10-.*\.conf', sub {
+	my ($file) = @_;
+	push @config_files, "$config_directory/$file";
+    });
+
+    unlink @config_files;
 }
 
 sub after_configure {
-- 
2.39.2

[pve-devel] applied: [PATCH pve-network] dhcp: dnsmasq: Use dir_glob_foreach for deleting configuration files

[Thomas Lamprecht]

Am 28/11/2023 um 09:58 schrieb Stefan Hanreich:
> The current invocation is quite unsafe and triggers the taint mode of
> Perl. Replacing it with dir_glob_foreach solves those issues.
> 
> Reported-By: Friedrich Weber <f.weber@proxmox.com>
> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> I wasn't sure whether directly unlinking the files in the callback
> would influence the iteration, hence why I store them in an
> intermediate array. Also, unlinking them all at once probably is
> better than unlinking them one-by-one (although it shouldn't matter
> with the low amount of files here..)

At least POSIX doesn't gives any guarantee:

> If a file is removed from or added to the directory after the most
> recent call to opendir() or rewinddir(), whether a subsequent call to
> readdir() returns an entry for that file is unspecified.

And Linux seems to only guarantee that files not added or removed since
the last opendir are returned, so it probably would work for this use
case, but IMO it's just not worth the hassle finding out if there's
some odd edge case, so a intermediate array was a good call.

> 
>  src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
>

applied, thanks!