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

wal: check out of range slice in "ReadAll", entry limits in "decodeRecord" #11793

Merged
merged 1 commit into from
Apr 23, 2020

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Apr 16, 2020

wal/decoder.go Outdated
@@ -79,6 +80,9 @@ func (d *decoder) decodeRecord(rec *walpb.Record) error {
}

recBytes, padBytes := decodeFrameSize(l)
if recBytes >= math.MaxInt64-padBytes {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we have a max wal size, also a max key-value pair size. i feel these can be set as a better limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we have a max wal size, also a max key-value pair size. i feel these can be set as a better limit.

Do we mean define one for max wal entry size? We have request size limit, but I don't see anywhere we define limit?

etcd/wal/wal.go

Lines 777 to 786 in 1166b1f

func (w *WAL) saveEntry(e *raftpb.Entry) error {
// TODO: add MustMarshalTo to reduce one allocation.
b := pbutil.MustMarshal(e)
rec := &walpb.Record{Type: entryType, Data: b}
if err := w.encoder.encode(rec); err != nil {
return err
}
w.enti = e.Index
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

we have a request limit. also we know that the throughput wont be unlimited. we probably will only be able to batch at most 1000 requests into one WAL entry at most, usually far less... So I guess a 2GB wal entry limit is large enough...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check for entry upper limit. Here I assume no project ever allows more than 10 MB raft message entry.

@gyuho gyuho changed the title wal: check out of range slice in "ReadAll" wal: check out of range slice in "ReadAll", entry limits in "decodeRecord" Apr 22, 2020
@@ -412,7 +415,12 @@ func (w *WAL) ReadAll() (metadata []byte, state raftpb.HardState, ents []raftpb.
case entryType:
e := mustUnmarshalEntry(rec.Data)
if e.Index > w.start.Index {
ents = append(ents[:e.Index-w.start.Index-1], e)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we simply check 0 <= e.Index-w.start.Index - 1 < len(ents)?

wal: add slice bound checks in decoder

CHANGELOG-3.5: add wal slice bound check
CHANGELOG-3.5: add "decodeRecord"

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
@codecov-io
Copy link

Codecov Report

Merging #11793 into master will decrease coverage by 0.68%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11793      +/-   ##
==========================================
- Coverage   66.69%   66.00%   -0.69%     
==========================================
  Files         403      403              
  Lines       36976    36981       +5     
==========================================
- Hits        24660    24411     -249     
- Misses      10822    11068     +246     
- Partials     1494     1502       +8     
Impacted Files Coverage Δ
wal/decoder.go 92.04% <0.00%> (-2.15%) ⬇️
wal/wal.go 56.77% <100.00%> (+0.70%) ⬆️
auth/store.go 53.37% <0.00%> (-24.44%) ⬇️
proxy/grpcproxy/register.go 72.50% <0.00%> (-10.00%) ⬇️
auth/simple_token.go 79.83% <0.00%> (-9.25%) ⬇️
client/members.go 65.32% <0.00%> (-8.88%) ⬇️
clientv3/leasing/util.go 91.66% <0.00%> (-6.67%) ⬇️
clientv3/namespace/watch.go 87.87% <0.00%> (-6.07%) ⬇️
etcdserver/api/v3rpc/member.go 87.09% <0.00%> (-3.23%) ⬇️
clientv3/leasing/txn.go 88.09% <0.00%> (-3.18%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6aea6ed...c716d76. Read the comment docs.

@xiang90
Copy link
Contributor

xiang90 commented Apr 23, 2020

lgtm

Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

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

lgtm Thanks @gyuho

@spzala spzala merged commit f4b650b into etcd-io:master Apr 23, 2020
@gyuho gyuho deleted the fix branch April 23, 2020 20:32
gyuho added a commit that referenced this pull request Jun 25, 2020
…-upstream-release-3.4

Automated cherry pick of #11793
gyuho added a commit that referenced this pull request Jun 26, 2020
…-upstream-release-3.3

Automated cherry pick of #11793
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants