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

endpoint: add recursion limit check #2432

Merged
merged 4 commits into from Nov 5, 2017

Conversation

Projects
None yet
3 participants
@BusyJay
Contributor

BusyJay commented Oct 28, 2017

This is a rewrite of pr #2424. Instead of checking it in executors, check it at the very beginning of deserialization.

Because pr stepancheg/rust-protobuf#248 is not merged (and released) yet, so use a branch for now.

@BusyJay BusyJay requested review from siddontang, overvenus and disksing Oct 28, 2017

Show outdated Hide outdated Cargo.toml
RequestTask::new(
req,
box move |res| {
let s = format!("{:?}", res);

This comment has been minimized.

@siddontang

siddontang Oct 29, 2017

Contributor

better to check error and then the error msg.

@siddontang

siddontang Oct 29, 2017

Contributor

better to check error and then the error msg.

This comment has been minimized.

@BusyJay

BusyJay Oct 29, 2017

Contributor

The type of res is a protobuf message, no way to check the exact type of the error.

@BusyJay

BusyJay Oct 29, 2017

Contributor

The type of res is a protobuf message, no way to check the exact type of the error.

for _ in 0..10 {
let mut e = Expr::new();
e.mut_children().push(expr);
expr = e;

This comment has been minimized.

@siddontang

siddontang Oct 29, 2017

Contributor

can height 10 cause stack > 100?

@siddontang

siddontang Oct 29, 2017

Contributor

can height 10 cause stack > 100?

This comment has been minimized.

@BusyJay

BusyJay Oct 29, 2017

Contributor

Maybe and may not. The check here is not exact the level of stack but just the nested level of messages.

@BusyJay

BusyJay Oct 29, 2017

Contributor

Maybe and may not. The check here is not exact the level of stack but just the nested level of messages.

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Oct 29, 2017

Contributor

LGTM

Contributor

siddontang commented Oct 29, 2017

LGTM

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang
Contributor

siddontang commented Oct 30, 2017

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Oct 31, 2017

Contributor

PTAL @hicqu

Contributor

siddontang commented Oct 31, 2017

PTAL @hicqu

@overvenus

This comment has been minimized.

Show comment
Hide comment
@overvenus

overvenus Oct 31, 2017

Member

Why not update etc/config-template.toml?

Member

overvenus commented Oct 31, 2017

Why not update etc/config-template.toml?

@overvenus

LGTM

@siddontang

LGTM

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Nov 2, 2017

Contributor

CI failed @BusyJay

Contributor

siddontang commented Nov 2, 2017

CI failed @BusyJay

@siddontang siddontang merged commit 58a47de into tikv:master Nov 5, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

zhangjinpeng1987 added a commit to zhangjinpeng1987/tikv that referenced this pull request Dec 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment