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

stdlib._net_http_ResponseWriter is not seen as an http.Hijacker #1425

Closed
mpl opened this issue Jul 4, 2022 · 2 comments · Fixed by #1443
Closed

stdlib._net_http_ResponseWriter is not seen as an http.Hijacker #1425

mpl opened this issue Jul 4, 2022 · 2 comments · Fixed by #1443
Labels
area/core bug Something isn't working

Comments

@mpl
Copy link
Collaborator

mpl commented Jul 4, 2022

The following program sample.go triggers an unexpected result

package main

import (
	"bufio"
	"errors"
	"log"
	"net"
	"net/http"
	"net/http/httputil"
	"net/url"
)

type Middleware struct {
	next http.Handler
}

func (h *Middleware) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
	wrw := &WrappedWriter{writer: rw}
	h.next.ServeHTTP(wrw, r)
}

func main() {
	// forwards to e.g. websocat -s 6060
	dest, err := url.Parse("http://localhost:6060")
	if err != nil {
		log.Fatal(err)
	}
	next := httputil.NewSingleHostReverseProxy(dest)
	http.Handle("/", &Middleware{next})
	log.Fatal(http.ListenAndServe(":8080", nil))
}

type WrappedWriter struct {
	writer http.ResponseWriter
}

func (w *WrappedWriter) Header() http.Header {
	return w.writer.Header()
}

func (w *WrappedWriter) Write(buf []byte) (int, error) {
	return w.writer.Write(buf)
}

func (w *WrappedWriter) WriteHeader(statusCode int) {
	w.writer.WriteHeader(statusCode)
}

func (w *WrappedWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
	h, ok := w.writer.(http.Hijacker)
	if !ok {
		return nil, nil, errors.New("hijack not supported")
	}
	return h.Hijack()
}

Expected result

As a backend, run e.g. (from https://github.com/vi/websocat , there's a brew for it):
$ websocat -s 6060

Start the above wrapped reverse proxy:
$ go run ./sample.go

Issue a websocket request on the reverse proxy (which will trigger the hijacking):
$ websocat ws://127.0.0.1:8080/
hello
("hello" should be printed on the backend output)

Got

$ yaegi run ./sample.go

$ websocat ws://127.0.0.1:8080/
websocat: WebSocketError: Received unexpected status code (502 Bad Gateway)
websocat: error running

Error on the yaegi side:
reverseproxy.go:490: http: proxy error: can't switch protocols using non-Hijacker ResponseWriter type stdlib._net_http_ResponseWriter

Yaegi Version

cb642c4 (devel)

Additional Notes

Related traefik issue:

traefik/traefik#8682

@mvertes mvertes added bug Something isn't working area/core labels Jul 5, 2022
@mvertes mvertes added this to the v0.13.x milestone Jul 5, 2022
@mpl
Copy link
Collaborator Author

mpl commented Jul 8, 2022

Another example, from the same root cause, but with a different impact, and in a different part of the stdlib.
Realistically, in this case it means yaegi probably wouldn't break the big picture behaviour (if the user implementation of WriteTo was honest and not wonky like mine), but it definitely would break some potential optimizations.

package main

import (
	"io"
	"log"
	"os"
	"strings"
)

type WrappedReader struct {
	reader io.Reader
}

func (wr WrappedReader) Read(p []byte) (n int, err error) {
	return wr.reader.Read(p)
}

// Of course, this implementation is completely stupid because it does not write
// to the intended writer, as any honest WriteTo implementation should. its
// implemtion is just to make obvious the divergence of behaviour with yaegi.
func (wr WrappedReader) WriteTo(w io.Writer) (n int64, err error) {
	// Ignore w, send to Stdout to prove whether this WriteTo is used.
	data, err := io.ReadAll(wr)
	if err != nil {
		return 0, err
	}
	nn, err := os.Stdout.Write(data)
	return int64(nn), err
}

func main() {
	f := strings.NewReader("hello world")
	wr := WrappedReader{reader: f}

	// behind the scenes, io.Copy is supposed to use wr.WriteTo if the implementation exists.
	// With Go, it works as expected, i.e. the output is sent to os.Stdout.
	// With Yaegi, it doesn't, i.e. the output is sent to io.Discard.
	if _, err := io.Copy(io.Discard, wr); err != nil {
		log.Fatal(err)
	}
}

@mpl
Copy link
Collaborator Author

mpl commented Jul 11, 2022

Another different/interesting case:

package main

import (
	"image"
	"io"
	"log"
	"os"
)

type WrappedReader struct {
	reader io.Reader
}

func (wr WrappedReader) Read(p []byte) (n int, err error) {
	return wr.reader.Read(p)
}

func (wr WrappedReader) Peek(i int) ([]byte, error) {
	println("BLABLA")
	return []byte("hello"), nil
}

func main() {
	f := strings.NewReader("hello world")
	wr := WrappedReader{reader: f}
	_, format, err := image.Decode(wr)
	if err != nil {
		log.Fatal(err)
	}
	println(format)
}

this one is a little bit different, because the implemented interface is actually a non-exported one this time, in image/format.go:

// A reader is an io.Reader that can also peek ahead.
type reader interface {
	io.Reader
	Peek(int) ([]byte, error)
}

which is used in e.g. asReader, which is called in Decode.

As with the other cases above, the Go compiler sees the interface is being implemented, whereas Yaegi does not.

Edit:

I went through the rest of the image/* package, and I believe these are the other interfaces we'll have to address as well:

in jpeg/writer.go and gif/writer.go , we have the exact counterpart of the reader case above, i.e.:

// writer is a buffered writer.
type writer interface {
	Flush() error
	io.Writer
	io.ByteWriter
}

which is used when encoding images.
Amusingly/interestingly it does not exist for pngs.

On the other hand, we have specific interfaces used in png:

type opaquer interface {
	Opaque() bool
}

In theory, we need to take care of opaquer, because it is asserted when encoding from an image.Image passed as argument, as so:

	if o, ok := m.(opaquer); ok {
		return o.Opaque()
	}

but in practice it is unlikely to be a problem, since the image passed as argument is usually a concrete type coming from the stdlib (e.g. type RGBA) too, which itself implements the Opaque method.

And it's more or less the same situation with:

type PalettedImage interface {
	// ColorIndexAt returns the palette index of the pixel at (x, y).
	ColorIndexAt(x, y int) uint8
	Image
}

@ldez ldez modified the milestones: v0.13.x, v0.14.x Aug 3, 2022
mvertes added a commit to mvertes/yaegi that referenced this issue Aug 23, 2022
This change implements a workaround to better support composed
interfaces in yaegi and let the interpreter to define objects which
impletment multiple interfaces at once.

We use the existing MapTypes to store what possible composed interfaces
wrapper could be used for some interfaces. When generating an interface
wrapper, the wrapper with the highest number of implemented methods is
chosen.

This is still an imperfect solution but it improves the accuracy of
interpreter in some critical cases.

This workaround could be removed in future if/when golang/go#15924
is resolved.

Fixes traefik#1425.
traefiker pushed a commit that referenced this issue Aug 25, 2022
This change implements a workaround to better support composed
interfaces in yaegi and let the interpreter define objects which
implement multiple interfaces at once.

We use the existing MapTypes to store what possible composed interface
wrapper could be used for some interfaces. When generating an interface
wrapper, the wrapper with the highest number of implemented methods is
chosen.

This is still an imperfect solution but it improves the accuracy of
interpreter in some critical cases.

This workaround could be removed in future if/when golang/go#15924
is resolved.

Fixes #1425.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants