Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Baggage propagation - replace invalid percent-encoded octet sequnces with replacement char #5519

Closed
lachmatt opened this issue Jun 18, 2024 · 5 comments
Assignees
Labels
area:baggage Part of OpenTelemetry baggage bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@lachmatt
Copy link

lachmatt commented Jun 18, 2024

W3C baggage spec requires:

When decoding the value, percent-encoded octet sequences that do not match the UTF-8 encoding scheme MUST be replaced with the replacement code point (U+FFFD).

This situation is currently not handled correctly - see playground for sample:

package main

import (
	"fmt"
	"log"
	"unicode/utf8"

	"go.opentelemetry.io/otel/baggage"
)

func main() {
	kv := "k=aa%ffcc"

	b, err := baggage.Parse(kv)
	if err != nil {
		log.Fatal(err)
	}
	val := b.Members()[0].Value()

	fmt.Println(val)
	fmt.Println(len(val))
	fmt.Println(utf8.ValidString(val))

	validStringWithReplacements := "aa�cc"
	fmt.Println(len(validStringWithReplacements))
	fmt.Println(utf8.ValidString(validStringWithReplacements))
}
@lachmatt lachmatt added the bug Something isn't working label Jun 18, 2024
@pellared pellared added area:baggage Part of OpenTelemetry baggage good first issue Good for newcomers help wanted Extra attention is needed labels Jun 18, 2024
@santileira
Copy link
Contributor

hey @lachmatt what is the expected behavior?

that code should return 7 and true, right?

fmt.Println(len(val))
fmt.Println(utf8.ValidString(val))

@pellared
Copy link
Member

pellared commented Jun 19, 2024

that code should return 7 and true

This is correct.

From https://www.compart.com/en/unicode/U+FFFD:

UTF-8 Encoding: 0xEF 0xBF 0xBD

@santileira
Copy link
Contributor

@pellared I could fix it, could you assign me this issue?

dmathieu added a commit that referenced this issue Jul 8, 2024
# Goal

Replace the percent encoded octet sequence with the replacement code
point (U+FFFD) when it doesn't match the UTF-8 encoding schema.

Issue: #5519

Current behavior: 

```
package main

import (
	"fmt"
	"log"
	"unicode/utf8"

	"go.opentelemetry.io/otel/baggage"
)

func main() {
	kv := "k=aa%ffcc"

	b, err := baggage.Parse(kv)
	if err != nil {
		log.Fatal(err)
	}
	val := b.Members()[0].Value()

	fmt.Println(len(val)) # 5
	fmt.Println(utf8.ValidString(val)) # false
}
```

Expected behavior:
```
package main

import (
	"fmt"
	"log"
	"unicode/utf8"

	"go.opentelemetry.io/otel/baggage"
)

func main() {
	kv := "k=aa%ffcc"

	b, err := baggage.Parse(kv)
	if err != nil {
		log.Fatal(err)
	}
	val := b.Members()[0].Value()

	fmt.Println(len(val)) # 7
	fmt.Println(utf8.ValidString(val)) # true
}
```

## Benchmark

- `go test -bench=BenchmarkParse -count 20 > old.txt`
```
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/baggage
BenchmarkParse-10    	 1548118	       774.3 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1547653	       786.0 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1544949	       770.5 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1558972	       770.2 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1554973	       774.7 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1550200	       779.6 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1545100	       774.3 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1549634	       777.5 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1552530	       769.6 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1536499	       855.0 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1552244	       770.4 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1560225	       767.4 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1562738	       772.3 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1556679	       838.9 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1562500	       777.1 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1530901	       836.5 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1000000	      1372 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1534678	       780.3 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1366180	       822.4 ns/op	     864 B/op	       8 allocs/op
BenchmarkParse-10    	 1539852	       796.8 ns/op	     864 B/op	       8 allocs/op
PASS
ok  	go.opentelemetry.io/otel/baggage	40.839s

```

- `go test -bench=BenchmarkParse -count 20 > new.txt`
```
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/baggage
BenchmarkParse-10    	 1355893	       886.6 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1349192	       883.1 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1363053	       880.4 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1372404	       875.7 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1359979	       880.7 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1360497	       874.7 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1375520	       870.2 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1375268	       882.8 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1361998	       964.8 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1373461	       961.5 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1378065	       872.6 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1377290	       879.0 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1362094	       885.6 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1352175	       915.9 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1364914	       887.9 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1355782	       890.5 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1361848	      1245 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1163396	       878.8 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1370886	       916.6 ns/op	     888 B/op	       9 allocs/op
BenchmarkParse-10    	 1340149	      1175 ns/op	     888 B/op	       9 allocs/op
PASS
ok  	go.opentelemetry.io/otel/baggage	44.347s
```

- `benchstat old.txt new.txt`
```
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/baggage
         │   old.txt   │               new.txt               │
         │   sec/op    │   sec/op     vs base                │
Parse-10   777.3n ± 3%   884.4n ± 4%  +13.77% (p=0.000 n=20)

         │  old.txt   │              new.txt              │
         │    B/op    │    B/op     vs base               │
Parse-10   864.0 ± 0%   888.0 ± 0%  +2.78% (p=0.000 n=20)

         │  old.txt   │              new.txt               │
         │ allocs/op  │ allocs/op   vs base                │
Parse-10   8.000 ± 0%   9.000 ± 0%  +12.50% (p=0.000 n=20)
```

---------

Co-authored-by: Damien Mathieu <42@dmathieu.com>
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@santileira
Copy link
Contributor

@pellared / @dmathieu We already merged the code to solve this issue, should we close the issue? Thanks

@dmathieu dmathieu closed this as completed Jul 8, 2024
@dmathieu
Copy link
Member

dmathieu commented Jul 8, 2024

Fixed in #5528.

@XSAM XSAM added this to the untracked milestone Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:baggage Part of OpenTelemetry baggage bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
Status: Closed
Development

No branches or pull requests

5 participants