Handle empty buffers in resp parsing correctly, return false for array check and EUNFIN for general parsing #161
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seppo0010
reviewed
May 3, 2018
| @@ -186,7 +186,7 @@ token_is_array(struct buf *buf) | |||
| { | |||
| ASSERT(buf != NULL); | |||
|
|
|||
| return *(buf->rpos) == '*'; | |||
| return (buf_rsize(buf) > 0 && *(buf->rpos) == '*'); | |||
There was a problem hiding this comment.
It makes more sense to return parse_rstatus_t here since both callers are validating the size of the read buffer...
how about something like this?
parse_rstatus_t
token_is_array(struct buf *buf)
{
ASSERT(buf != NULL)
if (buf_rsize(buf) == 0) {
return PARSE_EUNFIN;
}
if (*(buf->rpos) == '*') {
return PARSE_OK;
}
return PARSE_EINVALID;
}And callers should
// ...
parse_rstatus_t status = token_is_array(buf);
if (status != PARSE_OK) {
return status;
}
// ...
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
slyphon
added a commit
to slyphon/pelikan
that referenced
this pull request
Jul 20, 2018
6b59401 Rust build tweak (twitter#165) b39086d fix docs to new ring_array_destroy semantics (twitter#163) 95e04b0 Additional ring_array API (twitter#159) 3c994cc a single job in the travis-ci matrix for building with rust enabled (twitter#161) 1fe907e rust support - Native bstrings (twitter#160) b3de2ee add HAVE_RUST option defaulted to off (twitter#158) ffc70a8 Port travis ci changes - closes twitter#186 (twitter#155) a2d5c01 add a function that will reject all pending incoming connections (twitter#154) dd56abd break up ASSERT in buf_sock_read (twitter#153) 727905b adding a new API to duration timer (twitter#152) 9406717 add missing "extern C" qualifiers to a few headers for C++ linking; (twitter#150) a0aafdf Add missing semicolon to ring array example code in documentation. (twitter#149) 9264bbb Zero byte (twitter#147) (emergency fix needed for pelikan) d4002d7 simplify cc_print_int64 (twitter#146) b164fcf Clean-up hash functions and introduce MurmurHash3 (twitter#145) b1babb2 change wheel's sleep timer to make it less flaky (twitter#143) ce0b9ea allow printing negative integers in cc_print (twitter#141) ab0edc8 add metrics to track buf_sock objects (twitter#138) ae02038 add travis ci (copied from pelikan) (twitter#139) 964645a Merge pull request twitter#135 from paegun/fix_cmake_install 70710c2 fixed and re-added cmake install instructions, w/ following notes: include directory made proper relative; opened pattern match b/c include directory should only contain files meant for inclusion. 5b095bc Merge pull request twitter#126 from kevyang/kevyang/120 426d56a return NULL when cc_alloc/cc_realloc is called with size == 0 ad271d4 Merge pull request twitter#133 from kevyang/132 47dbdba suppress unused parameter warning in debug_log_flush 648d19e Merge pull request twitter#127 from kevyang/56 780941a Merge pull request twitter#130 from kevyang/129 b8af6c0 Merge pull request twitter#131 from kevyang/128 6ecc318 fix duplicate symbols in cc_signal 080c41d cc_array - stop doing arithmetic on void * d526f7a add debug oriented memory management a4fb927 Update bool member rules in style guide 05c6e1e explicitly make ccommon a C project to avoid checking for CXX related variables git-subtree-dir: deps/ccommon git-subtree-split: 6b59401
slyphon
added a commit
that referenced
this pull request
Jul 21, 2018
f5efe29 forgot to remove unused include (#167) 441934b update ring_array docs for new API, add multi threaded unit test for ring_array (#166) 67ce9c2 Adding issue and pull request templates to comply with twitter OSS policies (#151) 6b59401 Rust build tweak (#165) b39086d fix docs to new ring_array_destroy semantics (#163) 95e04b0 Additional ring_array API (#159) 3c994cc a single job in the travis-ci matrix for building with rust enabled (#161) 1fe907e rust support - Native bstrings (#160) b3de2ee add HAVE_RUST option defaulted to off (#158) ffc70a8 Port travis ci changes - closes #186 (#155) a2d5c01 add a function that will reject all pending incoming connections (#154) dd56abd break up ASSERT in buf_sock_read (#153) 727905b adding a new API to duration timer (#152) 9406717 add missing "extern C" qualifiers to a few headers for C++ linking; (#150) a0aafdf Add missing semicolon to ring array example code in documentation. (#149) 9264bbb Zero byte (#147) (emergency fix needed for pelikan) d4002d7 simplify cc_print_int64 (#146) b164fcf Clean-up hash functions and introduce MurmurHash3 (#145) b1babb2 change wheel's sleep timer to make it less flaky (#143) ce0b9ea allow printing negative integers in cc_print (#141) ab0edc8 add metrics to track buf_sock objects (#138) ae02038 add travis ci (copied from pelikan) (#139) 964645a Merge pull request #135 from paegun/fix_cmake_install 70710c2 fixed and re-added cmake install instructions, w/ following notes: include directory made proper relative; opened pattern match b/c include directory should only contain files meant for inclusion. 5b095bc Merge pull request #126 from kevyang/kevyang/120 426d56a return NULL when cc_alloc/cc_realloc is called with size == 0 ad271d4 Merge pull request #133 from kevyang/132 47dbdba suppress unused parameter warning in debug_log_flush 648d19e Merge pull request #127 from kevyang/56 780941a Merge pull request #130 from kevyang/129 b8af6c0 Merge pull request #131 from kevyang/128 6ecc318 fix duplicate symbols in cc_signal 080c41d cc_array - stop doing arithmetic on void * d526f7a add debug oriented memory management a4fb927 Update bool member rules in style guide 05c6e1e explicitly make ccommon a C project to avoid checking for CXX related variables git-subtree-dir: deps/ccommon git-subtree-split: f5efe29
swlynch99
pushed a commit
to swlynch99/pelikan-twitter
that referenced
this pull request
Sep 30, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.