[pve-devel] [PATCH common/cluster 0/3] fix #4937: fix utf8 encoding issues while saving notification config

[pve-devel] [PATCH common/cluster 0/3] fix #4937: fix utf8 encoding issues while saving notification config

[Lukas Wagner]

These patches should fix issues with certain special characters
(e.g ü) in the notification configuration [1].
Before, when setting a comment for an endpoint to certain values 
(e.g. 'für admins'), the resulting saved configuration would 
contain invalid UTF8 text.

The reason for that was the configuration was returned from
Rust as a proper utf8 string, however it was saved to the file 
system without specifying an encoding.


[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4937



pve-common:

Lukas Wagner (1):
  tools: allow to specify file encoding for file_set_contents

 src/PVE/Tools.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


pve-cluster:

Lukas Wagner (2):
  cluster fs: allow to specify file encoding for cfs_write_file
  fix #4937: notify: write configuration files in utf8 encoding

 src/PVE/Cluster.pm | 4 ++--
 src/PVE/Notify.pm  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)


Summary over all repositories:
  3 files changed, 10 insertions(+), 5 deletions(-)

-- 
murpp v0.4.0

[pve-devel] [PATCH pve-common 1/3] tools: allow to specify file encoding for file_set_contents

[Lukas Wagner]

Rationale: This is used from cfs_write_file, which is now also used to
write utf8-encoded strings that come from Rust. If no encoding is
specified while writing the file, we run into problems with certain
special characters (e.g. 'ü')

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/PVE/Tools.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 50240c8..f7224f8 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -237,7 +237,7 @@ sub lock_file {
 }
 
 sub file_set_contents {
-    my ($filename, $data, $perm)  = @_;
+    my ($filename, $data, $perm, $encoding)  = @_;
 
     $perm = 0644 if !defined($perm);
 
@@ -247,6 +247,11 @@ sub file_set_contents {
 	my ($fh, $tries) = (undef, 0);
 	while (!$fh && $tries++ < 3) {
 	    $fh = IO::File->new($tmpname, O_WRONLY|O_CREAT|O_EXCL, $perm);
+
+	    if (defined($encoding)) {
+		binmode($fh, ":encoding($encoding)");
+	    }
+
 	    if (!$fh && $! == EEXIST) {
 		unlink($tmpname) or die "unable to delete old temp file: $!\n";
 	    }
-- 
2.39.2

[pve-devel] [PATCH pve-cluster 2/3] cluster fs: allow to specify file encoding for cfs_write_file

[Lukas Wagner]

Since this function is used to store utf8-encoded strings that come
from Rust, we need to be able to save the file in proper utf8 encoding
as well.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/PVE/Cluster.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Cluster.pm b/src/PVE/Cluster.pm
index e3705b6..9fdc23d 100644
--- a/src/PVE/Cluster.pm
+++ b/src/PVE/Cluster.pm
@@ -578,7 +578,7 @@ sub cfs_read_file {
 }
 
 sub cfs_write_file {
-    my ($filename, $data) = @_;
+    my ($filename, $data, $encoding) = @_;
 
     my ($version, $info) = cfs_file_version($filename);
 
@@ -592,7 +592,7 @@ sub cfs_write_file {
 	$ci->{version} = undef;
     }
 
-    PVE::Tools::file_set_contents($fsname, $raw);
+    PVE::Tools::file_set_contents($fsname, $raw, undef, $encoding);
 }
 
 my $cfs_lock = sub {
-- 
2.39.2

[pve-devel] [PATCH pve-cluster 3/3] fix #4937: notify: write configuration files in utf8 encoding

[Lukas Wagner]

Strings that are returned from the Rust implementation are encoded as
utf8. To avoid issues with certain special characters (e.g. german
umlauts), we also need to explicitly store the configuration files in
utf8 encoding.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/PVE/Notify.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Notify.pm b/src/PVE/Notify.pm
index 0464362..d393a41 100644
--- a/src/PVE/Notify.pm
+++ b/src/PVE/Notify.pm
@@ -79,8 +79,8 @@ sub write_config {
     };
 
     my ($config, $priv_config) = $notification_config->write_config();
-    cfs_write_file('notifications.cfg', $config);
-    cfs_write_file('priv/notifications.cfg', $priv_config);
+    cfs_write_file('notifications.cfg', $config, "utf8");
+    cfs_write_file('priv/notifications.cfg', $priv_config, "utf8");
 }
 
 sub default_target {
-- 
2.39.2

Re: [pve-devel] [PATCH common/cluster 0/3] fix #4937: fix utf8 encoding issues while saving notification config

[Lukas Wagner]

On 8/30/23 14:37, Lukas Wagner wrote:
> These patches should fix issues with certain special characters
> (e.g ü) in the notification configuration [1].
> Before, when setting a comment for an endpoint to certain values
> (e.g. 'für admins'), the resulting saved configuration would
> contain invalid UTF8 text.
> 
> The reason for that was the configuration was returned from
> Rust as a proper utf8 string, however it was saved to the file
> system without specifying an encoding.
> 
> 
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4937

Since I'm on vacation for the next two weeks:
These changes should be applied before the new notification system hits 
the no-subscription repo (pve-manager 8.0.5), since this bug can break
notifications entirely (to fix it, one has to delete notifications.cfg 
and priv/notifications.cfg, or at least the faulty characters from these 
files)

-- 
- Lukas

[pve-devel] applied-series: [PATCH common/cluster 0/3] fix #4937: fix utf8 encoding issues while saving notification config

[Fiona Ebner]

Am 30.08.23 um 14:37 schrieb Lukas Wagner:
> These patches should fix issues with certain special characters
> (e.g ü) in the notification configuration [1].
> Before, when setting a comment for an endpoint to certain values 
> (e.g. 'für admins'), the resulting saved configuration would 
> contain invalid UTF8 text.
> 
> The reason for that was the configuration was returned from
> Rust as a proper utf8 string, however it was saved to the file 
> system without specifying an encoding.
> 
> 
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=4937
> 

applied-series, after discussing with Fabian, thanks!

We decided to make it a $force_utf8 flag instead of passing the encoding
as a string, and use the strict 'UTF-8' encoding rather than 'utf8'. See
the "UTF-8 vs. utf8 vs. UTF8" section in 'perldoc Encode' for more info.
I also moved the binmode() call to after the loop, where $fh is known to
be set.

We also found that file_get_contents() doesn't set Perl's UTF-8 flag for
strings even if the input is UTF-8, because it uses sysread(). It's fine
for the notification configs apparently, but going forward, we'll need
to be careful. One idea was an opt-in parameter for detecting UTF-8 and
setting the Perl string flag and using that for (most) cluster
filesystem files.