Skip to content

context: adjust the file write logic to avoid corrupt context meta.json files #4042

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.

Sign up for GitHub

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

Merged
thaJeztah merged 1 commit into from
Feb 23, 2023
Merged

context: adjust the file write logic to avoid corrupt context meta.json files #4042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions cli/context/store/metadatastore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ package store

import (
"encoding/json"
"fmt"
"os"
"path/filepath"
"reflect"
"sort"

"github.com/docker/docker/errdefs"
"github.com/docker/docker/pkg/ioutils"
"github.com/fvbommel/sortorder"
"github.com/pkg/errors"
)
Expand Down Expand Up @@ -35,7 +37,7 @@ func (s *metadataStore) createOrUpdate(meta Metadata) error {
if err != nil {
return err
}
return os.WriteFile(filepath.Join(contextDir, metaFile), bytes, 0o644)
return ioutils.AtomicWriteFile(filepath.Join(contextDir, metaFile), bytes, 0o644)
}

func parseTypedOrMap(payload []byte, getter TypeGetter) (interface{}, error) {
Expand Down Expand Up @@ -65,7 +67,8 @@ func (s *metadataStore) get(name string) (Metadata, error) {
}

func (s *metadataStore) getByID(id contextdir) (Metadata, error) {
bytes, err := os.ReadFile(filepath.Join(s.contextDir(id), metaFile))
fileName := filepath.Join(s.contextDir(id), metaFile)
bytes, err := os.ReadFile(fileName)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return Metadata{}, errdefs.NotFound(errors.Wrap(err, "context not found"))
Expand All @@ -77,15 +80,15 @@ func (s *metadataStore) getByID(id contextdir) (Metadata, error) {
Endpoints: make(map[string]interface{}),
}
if err := json.Unmarshal(bytes, &untyped); err != nil {
return Metadata{}, err
return Metadata{}, fmt.Errorf("parsing %s: %v", fileName, err)
Comment on lines -80 to +83
Copy link
Member

@thaJeztah thaJeztah Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes is masking the original error (%v). I don't THINK we use any of these errors as sentinel errors (had a cursory glance at call-sites for this code, and I think we only use the "not found" case as special), so we probably should be fine, but I'd 🤦 if I didn't comment, and regret it later.

I guess there's not much we can do in these cases, other than report "something was wrong; here's some info", so I guess we should be fine.

Only alternative would be to make this an errdefs.InvalidParam() (if we consider this "input is invalid", and to consistently return on of the errdefs types), but probably something we could still consider in a follow-up if we see a need.

}
r.Name = untyped.Name
if r.Metadata, err = parseTypedOrMap(untyped.Metadata, s.config.contextType); err != nil {
return Metadata{}, err
return Metadata{}, fmt.Errorf("parsing %s: %v", fileName, err)
}
for k, v := range untyped.Endpoints {
if r.Endpoints[k], err = parseTypedOrMap(v, s.config.endpointTypes[k]); err != nil {
return Metadata{}, err
return Metadata{}, fmt.Errorf("parsing %s: %v", fileName, err)
}
}
return r, err
Expand Down
27 changes: 27 additions & 0 deletions cli/context/store/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import (
"bytes"
"crypto/rand"
"encoding/json"
"fmt"
"io"
"os"
"path"
"path/filepath"
"testing"

"github.com/docker/docker/errdefs"
Expand Down Expand Up @@ -230,3 +232,28 @@ func TestImportZipInvalid(t *testing.T) {
err = Import("zipInvalid", s, r)
assert.ErrorContains(t, err, "unexpected context file")
}

func TestCorruptMetadata(t *testing.T) {
tempDir := t.TempDir()
s := New(tempDir, testCfg)
err := s.CreateOrUpdate(
Metadata{
Endpoints: map[string]interface{}{
"ep1": endpoint{Foo: "bar"},
},
Metadata: context{Bar: "baz"},
Name: "source",
})
assert.NilError(t, err)

// Simulate the meta.json file getting corrupted
// by some external process.
contextDir := s.meta.contextDir(contextdirOf("source"))
contextFile := filepath.Join(contextDir, metaFile)
err = os.WriteFile(contextFile, nil, 0o600)
assert.NilError(t, err)

// Assert that the error message gives the user some clue where to look.
_, err = s.GetMetadata("source")
assert.ErrorContains(t, err, fmt.Sprintf("parsing %s: unexpected end of JSON input", contextFile))
}
3 changes: 2 additions & 1 deletion cli/context/store/tlsstore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"path/filepath"

"github.com/docker/docker/errdefs"
"github.com/docker/docker/pkg/ioutils"
"github.com/pkg/errors"
)

Expand All @@ -31,7 +32,7 @@ func (s *tlsStore) createOrUpdate(name, endpointName, filename string, data []by
if err := os.MkdirAll(endpointDir, 0o700); err != nil {
return err
}
return os.WriteFile(filepath.Join(endpointDir, filename), data, 0o600)
return ioutils.AtomicWriteFile(filepath.Join(endpointDir, filename), data, 0o600)
}

func (s *tlsStore) getData(name, endpointName, filename string) ([]byte, error) {
Expand Down