From d856e87f4002fa520594a2c843cb7e3b002cbb6e Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 8 Jul 2020 13:50:52 +0200 Subject: events endpoint: fix panic and race condition Fix a potential panic in the events endpoint when parsing the filters parameter. Values of the filters map might be empty, so we need to account for that instead of uncondtitionally accessing the first item. Also apply a similar for race conditions as done in commit f4a2d25c0fca: Fix a race that could cause read errors to be masked. Masking such errors is likely to report red herrings since users don't see that reading failed for some reasons but that a given event could not be found. Another race was the handler closing event channel, which could lead to two kinds of panics: double close, send to close channel. The backend takes care of that. However, make sure that the backend stops working in case the context has been cancelled. Fixes: #6899 Signed-off-by: Valentin Rothberg --- pkg/api/handlers/compat/events.go | 92 +++++++++++++++++++++++---------------- pkg/bindings/test/system_test.go | 29 +++++++----- 2 files changed, 72 insertions(+), 49 deletions(-) (limited to 'pkg') diff --git a/pkg/api/handlers/compat/events.go b/pkg/api/handlers/compat/events.go index 5acc94153..3fc8248d6 100644 --- a/pkg/api/handlers/compat/events.go +++ b/pkg/api/handlers/compat/events.go @@ -1,9 +1,9 @@ package compat import ( - "context" "fmt" "net/http" + "sync" "github.com/containers/libpod/v2/libpod" "github.com/containers/libpod/v2/libpod/events" @@ -17,10 +17,10 @@ import ( func GetEvents(w http.ResponseWriter, r *http.Request) { var ( - fromStart bool - eventsError error - decoder = r.Context().Value("decoder").(*schema.Decoder) - runtime = r.Context().Value("runtime").(*libpod.Runtime) + fromStart bool + decoder = r.Context().Value("decoder").(*schema.Decoder) + runtime = r.Context().Value("runtime").(*libpod.Runtime) + json = jsoniter.ConfigCompatibleWithStandardLibrary // FIXME: this should happen on the package level ) query := struct { @@ -33,11 +33,16 @@ func GetEvents(w http.ResponseWriter, r *http.Request) { } if err := decoder.Decode(&query, r.URL.Query()); err != nil { utils.Error(w, "Failed to parse parameters", http.StatusBadRequest, errors.Wrapf(err, "Failed to parse parameters for %s", r.URL.String())) + return } var libpodFilters = []string{} if _, found := r.URL.Query()["filters"]; found { for k, v := range query.Filters { + if len(v) == 0 { + utils.Error(w, "Failed to parse parameters", http.StatusBadRequest, errors.Errorf("empty value for filter %q", k)) + return + } libpodFilters = append(libpodFilters, fmt.Sprintf("%s=%s", k, v[0])) } } @@ -46,46 +51,57 @@ func GetEvents(w http.ResponseWriter, r *http.Request) { fromStart = true } - eventCtx, eventCancel := context.WithCancel(r.Context()) eventChannel := make(chan *events.Event) + errorChannel := make(chan error) + + // Start reading events. go func() { - readOpts := events.ReadOptions{FromStart: fromStart, Stream: query.Stream, Filters: libpodFilters, EventChannel: eventChannel, Since: query.Since, Until: query.Until} - eventsError = runtime.Events(eventCtx, readOpts) + readOpts := events.ReadOptions{ + FromStart: fromStart, + Stream: query.Stream, + Filters: libpodFilters, + EventChannel: eventChannel, + Since: query.Since, + Until: query.Until, + } + errorChannel <- runtime.Events(r.Context(), readOpts) }() - if eventsError != nil { - utils.InternalServerError(w, eventsError) - eventCancel() - close(eventChannel) - return - } - // If client disappears we need to stop listening for events - go func(done <-chan struct{}) { - <-done - eventCancel() - if _, ok := <-eventChannel; ok { - close(eventChannel) - } - }(r.Context().Done()) + var coder *jsoniter.Encoder + var writeHeader sync.Once - // Headers need to be written out before turning Writer() over to json encoder - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - if flusher, ok := w.(http.Flusher); ok { - flusher.Flush() - } + for stream := true; stream; stream = query.Stream { + select { + case err := <-errorChannel: + if err != nil { + utils.InternalServerError(w, err) + return + } + case evt := <-eventChannel: + writeHeader.Do(func() { + // Use a sync.Once so that we write the header + // only once. + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + coder = json.NewEncoder(w) + coder.SetEscapeHTML(true) + }) - json := jsoniter.ConfigCompatibleWithStandardLibrary - coder := json.NewEncoder(w) - coder.SetEscapeHTML(true) + if evt == nil { + continue + } - for event := range eventChannel { - e := entities.ConvertToEntitiesEvent(*event) - if err := coder.Encode(e); err != nil { - logrus.Errorf("unable to write json: %q", err) - } - if flusher, ok := w.(http.Flusher); ok { - flusher.Flush() + e := entities.ConvertToEntitiesEvent(*evt) + if err := coder.Encode(e); err != nil { + logrus.Errorf("unable to write json: %q", err) + } + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } } + } } diff --git a/pkg/bindings/test/system_test.go b/pkg/bindings/test/system_test.go index 93141400b..430184f4a 100644 --- a/pkg/bindings/test/system_test.go +++ b/pkg/bindings/test/system_test.go @@ -1,6 +1,7 @@ package test_bindings import ( + "sync" "time" "github.com/containers/libpod/v2/pkg/bindings" @@ -38,22 +39,28 @@ var _ = Describe("Podman system", func() { }) It("podman events", func() { - eChan := make(chan entities.Event, 1) - var messages []entities.Event - cancelChan := make(chan bool, 1) + var name = "top" + _, err := bt.RunTopContainer(&name, bindings.PFalse, nil) + Expect(err).To(BeNil()) + + filters := make(map[string][]string) + filters["container"] = []string{name} + + binChan := make(chan entities.Event) + done := sync.Mutex{} + done.Lock() + eventCounter := 0 go func() { - for e := range eChan { - messages = append(messages, e) + defer done.Unlock() + for range binChan { + eventCounter++ } }() - go func() { - system.Events(bt.conn, eChan, cancelChan, nil, nil, nil, bindings.PFalse) - }() - _, err := bt.RunTopContainer(nil, nil, nil) + err = system.Events(bt.conn, binChan, nil, nil, nil, filters, bindings.PFalse) Expect(err).To(BeNil()) - cancelChan <- true - Expect(len(messages)).To(BeNumerically("==", 5)) + done.Lock() + Expect(eventCounter).To(BeNumerically(">", 0)) }) It("podman system prune - pod,container stopped", func() { -- cgit v1.2.3-54-g00ecf From dea93c27d2809cd5a04563d27cf00a8e682884c8 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 20 Jul 2020 14:44:05 +0200 Subject: events endpoint: backwards compat to old type The versions Docker that the compat endpoints currently support are using another type for the `filters` parameter than later versions of Docker, which the libpod/events endpoint is also using. To prevent existing deplopyments from breaking while still achieving backward compat, we now support both types for the filters parameter. Tested manually. Fixes: #6899 Signed-off-by: Valentin Rothberg --- pkg/api/handlers/compat/events.go | 70 ++++++++++++++++++++++++++++++--------- 1 file changed, 55 insertions(+), 15 deletions(-) (limited to 'pkg') diff --git a/pkg/api/handlers/compat/events.go b/pkg/api/handlers/compat/events.go index 3fc8248d6..9d5cb5045 100644 --- a/pkg/api/handlers/compat/events.go +++ b/pkg/api/handlers/compat/events.go @@ -1,6 +1,7 @@ package compat import ( + "encoding/json" "fmt" "net/http" "sync" @@ -15,6 +16,49 @@ import ( "github.com/sirupsen/logrus" ) +// filtersFromRequests extracts the "filters" parameter from the specified +// http.Request. The paramater can either be a `map[string][]string` as done +// in new versions of Docker and libpod, or a `map[string]map[string]bool` as +// done in older versions of Docker. We have to do a bit of Yoga to support +// both - just as Docker does as well. +// +// Please refer to https://github.com/containers/podman/issues/6899 for some +// background. +func filtersFromRequest(r *http.Request) ([]string, error) { + var ( + compatFilters map[string]map[string]bool + filters map[string][]string + libpodFilters []string + ) + raw := []byte(r.Form.Get("filters")) + + // Backwards compat with older versions of Docker. + if err := json.Unmarshal(raw, &compatFilters); err == nil { + for filterKey, filterMap := range compatFilters { + for filterValue, toAdd := range filterMap { + if toAdd { + libpodFilters = append(libpodFilters, fmt.Sprintf("%s=%s", filterKey, filterValue)) + } + } + } + return libpodFilters, nil + } + + if err := json.Unmarshal(raw, &filters); err != nil { + return nil, err + } + + for filterKey, filterSlice := range filters { + for _, filterValue := range filterSlice { + libpodFilters = append(libpodFilters, fmt.Sprintf("%s=%s", filterKey, filterValue)) + } + } + + return libpodFilters, nil +} + +// NOTE: this endpoint serves both the docker-compatible one and the new libpod +// one. func GetEvents(w http.ResponseWriter, r *http.Request) { var ( fromStart bool @@ -23,11 +67,12 @@ func GetEvents(w http.ResponseWriter, r *http.Request) { json = jsoniter.ConfigCompatibleWithStandardLibrary // FIXME: this should happen on the package level ) + // NOTE: the "filters" parameter is extracted separately for backwards + // compat via `fitlerFromRequest()`. query := struct { - Since string `schema:"since"` - Until string `schema:"until"` - Filters map[string][]string `schema:"filters"` - Stream bool `schema:"stream"` + Since string `schema:"since"` + Until string `schema:"until"` + Stream bool `schema:"stream"` }{ Stream: true, } @@ -36,21 +81,16 @@ func GetEvents(w http.ResponseWriter, r *http.Request) { return } - var libpodFilters = []string{} - if _, found := r.URL.Query()["filters"]; found { - for k, v := range query.Filters { - if len(v) == 0 { - utils.Error(w, "Failed to parse parameters", http.StatusBadRequest, errors.Errorf("empty value for filter %q", k)) - return - } - libpodFilters = append(libpodFilters, fmt.Sprintf("%s=%s", k, v[0])) - } - } - if len(query.Since) > 0 || len(query.Until) > 0 { fromStart = true } + libpodFilters, err := filtersFromRequest(r) + if err != nil { + utils.Error(w, "Failed to parse parameters", http.StatusBadRequest, errors.Wrapf(err, "Failed to parse parameters for %s", r.URL.String())) + return + } + eventChannel := make(chan *events.Event) errorChannel := make(chan error) -- cgit v1.2.3-54-g00ecf