chunked, composefs: fix decoding of xattrs

[giuseppe]

the value of the xattrs is encoded in base64, so decode them before passing the value to the mkcomposefs tool.

[cgwalters]

I think this error would have been more obvious if escaped() took a []byte right? That's what we have in the Rust code. The C code...well, a bit like Go, "byte array" vs "string" is a fluid, loosely enforced concept, but the presence there of a "length" member does signal it's binary.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[cgwalters]

(up to you whether or not to do my suggested change here or not, if you want to do it separately or not at all I can just lgtm)

[giuseppe]

I am fine to change it as part of this PR, if you think it is more readable.

Would amending the following patch be enough for what you had in mind?

diff --git a/pkg/chunked/dump/dump.go b/pkg/chunked/dump/dump.go
index 6293e3b7f..59d3622ab 100644
--- a/pkg/chunked/dump/dump.go
+++ b/pkg/chunked/dump/dump.go
@@ -23,12 +23,12 @@ const (
 	ESCAPE_LONE_DASH
 )
 
-func escaped(val string, escape int) string {
+func escaped(val []byte, escape int) string {
 	noescapeSpace := escape&NOESCAPE_SPACE != 0
 	escapeEqual := escape&ESCAPE_EQUAL != 0
 	escapeLoneDash := escape&ESCAPE_LONE_DASH != 0
 
-	if escapeLoneDash && val == "-" {
+	if escapeLoneDash && string(val) == "-" {
 		return fmt.Sprintf("\\x%.2x", val[0])
 	}
 
@@ -76,8 +76,8 @@ func escaped(val string, escape int) string {
 	return result
 }
 
-func escapedOptional(val string, escape int) string {
-	if val == "" {
+func escapedOptional(val []byte, escape int) string {
+	if string(val) == "" {
 		return "-"
 	}
 	return escaped(val, escape)
@@ -137,7 +137,7 @@ func dumpNode(out io.Writer, added map[string]*internal.FileMetadata, links map[
 	}
 	added[path] = entry
 
-	if _, err := fmt.Fprint(out, escaped(path, ESCAPE_STANDARD)); err != nil {
+	if _, err := fmt.Fprint(out, escaped([]byte(path), ESCAPE_STANDARD)); err != nil {
 		return err
 	}
 
@@ -181,7 +181,7 @@ func dumpNode(out io.Writer, added map[string]*internal.FileMetadata, links map[
 		}
 	}
 
-	if _, err := fmt.Fprint(out, escapedOptional(payload, ESCAPE_LONE_DASH)); err != nil {
+	if _, err := fmt.Fprint(out, escapedOptional([]byte(payload), ESCAPE_LONE_DASH)); err != nil {
 		return err
 	}
 
@@ -195,7 +195,7 @@ func dumpNode(out io.Writer, added map[string]*internal.FileMetadata, links map[
 		return err
 	}
 	digest := verityDigests[payload]
-	if _, err := fmt.Fprint(out, escapedOptional(digest, ESCAPE_LONE_DASH)); err != nil {
+	if _, err := fmt.Fprint(out, escapedOptional([]byte(digest), ESCAPE_LONE_DASH)); err != nil {
 		return err
 	}
 
@@ -204,9 +204,9 @@ func dumpNode(out io.Writer, added map[string]*internal.FileMetadata, links map[
 		if err != nil {
 			return fmt.Errorf("decode xattr %q: %w", k, err)
 		}
-		name := escaped(k, ESCAPE_EQUAL)
+		name := escaped([]byte(k), ESCAPE_EQUAL)
 
-		value := escaped(string(v), ESCAPE_EQUAL)
+		value := escaped(v, ESCAPE_EQUAL)
 		if _, err := fmt.Fprintf(out, " %s=%s", name, value); err != nil {
 			return err
 		}
diff --git a/pkg/chunked/dump/dump_test.go b/pkg/chunked/dump/dump_test.go
index 43584ac2e..d99693baf 100644
--- a/pkg/chunked/dump/dump_test.go
+++ b/pkg/chunked/dump/dump_test.go
@@ -38,7 +38,7 @@ func TestEscaped(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.input, func(t *testing.T) {
-			result := escaped(test.input, test.escape)
+			result := escaped([]byte(test.input), test.escape)
 			if result != test.want {
 				t.Errorf("got %q, want %q", result, test.want)
 			}

[cgwalters]

if escapeLoneDash && string(val) == "-" {

We can avoid the new allocation of a string here by comparing with '-' right?

[giuseppe]

if escapeLoneDash && string(val) == "-" {

We can avoid the new allocation of a string here by comparing with '-' right?

yes sure, fixed and amended the PR

[cgwalters]

/lgtm