From 344e866c44cafcd03b05c1f48d34eb98ee876322 Mon Sep 17 00:00:00 2001 From: yeoncheol-kim Date: Thu, 7 Mar 2024 15:23:09 +0900 Subject: [PATCH] FIX: Modify mismatched error types when get_item_info failed --- memcached.c | 165 +++++++++++++++++++++++++--------------------------- 1 file changed, 80 insertions(+), 85 deletions(-) diff --git a/memcached.c b/memcached.c index 75a6957ac..d47f351f4 100644 --- a/memcached.c +++ b/memcached.c @@ -3212,16 +3212,13 @@ static void complete_update_ascii(conn *c) } item *it = c->item; + ENGINE_ERROR_CODE ret; if (!mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) { - mc_engine.v1->release(mc_engine.v0, c, it); mc_logger->log(EXTENSION_LOG_WARNING, c, "%d: Failed to get item info\n", c->sfd); - out_string(c, "SERVER_ERROR failed to get item details"); - return; - } - - ENGINE_ERROR_CODE ret; - if (hinfo_check_ascii_tail_string(&c->hinfo) != 0) { /* check "\r\n" */ + out_string(c, "SERVER_ERROR out of memory for getting item info"); + ret = ENGINE_ENOMEM; + } else if (hinfo_check_ascii_tail_string(&c->hinfo) != 0) { /* check "\r\n" */ out_string(c, "CLIENT_ERROR bad data chunk"); ret = ENGINE_EBADVALUE; } else { @@ -3705,87 +3702,86 @@ static void complete_update_bin(conn *c) assert(c != NULL); item *it = c->item; + ENGINE_ERROR_CODE ret; if (!mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) { - mc_engine.v1->release(mc_engine.v0, c, it); mc_logger->log(EXTENSION_LOG_WARNING, c, "%d: Failed to get item info\n", c->sfd); - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINTERNAL, 0); - return; - } - /* We don't actually receive the trailing two characters in the bin - * protocol, so we're going to just append them here */ - hinfo_append_ascii_tail_string(&c->hinfo); /* append "\r\n" */ + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0); + ret = ENGINE_ENOMEM; + } else { + /* We don't actually receive the trailing two characters in the bin + * protocol, so we're going to just append them here */ + hinfo_append_ascii_tail_string(&c->hinfo); /* append "\r\n" */ - ENGINE_ERROR_CODE ret; - ret = mc_engine.v1->store(mc_engine.v0, c, it, &c->cas, c->store_op, - c->binary_header.request.vbucket); - CONN_CHECK_AND_SET_EWOULDBLOCK(ret, c); + ret = mc_engine.v1->store(mc_engine.v0, c, it, &c->cas, c->store_op, + c->binary_header.request.vbucket); + CONN_CHECK_AND_SET_EWOULDBLOCK(ret, c); #ifdef ENABLE_DTRACE - switch (c->cmd) { - case OPERATION_ADD: - MEMCACHED_COMMAND_ADD(c->sfd, c->hinfo.key, c->hinfo.nkey, - (ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas); - break; - case OPERATION_REPLACE: - MEMCACHED_COMMAND_REPLACE(c->sfd, c->hinfo.key, c->hinfo.nkey, + switch (c->cmd) { + case OPERATION_ADD: + MEMCACHED_COMMAND_ADD(c->sfd, c->hinfo.key, c->hinfo.nkey, (ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas); - break; - case OPERATION_APPEND: - MEMCACHED_COMMAND_APPEND(c->sfd, c->hinfo.key, c->hinfo.nkey, - (ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas); - break; - case OPERATION_PREPEND: - MEMCACHED_COMMAND_PREPEND(c->sfd, c->hinfo.key, c->hinfo.nkey, + break; + case OPERATION_REPLACE: + MEMCACHED_COMMAND_REPLACE(c->sfd, c->hinfo.key, c->hinfo.nkey, + (ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas); + break; + case OPERATION_APPEND: + MEMCACHED_COMMAND_APPEND(c->sfd, c->hinfo.key, c->hinfo.nkey, + (ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas); + break; + case OPERATION_PREPEND: + MEMCACHED_COMMAND_PREPEND(c->sfd, c->hinfo.key, c->hinfo.nkey, + (ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas); + break; + case OPERATION_SET: + MEMCACHED_COMMAND_SET(c->sfd, c->hinfo.key, c->hinfo.nkey, (ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas); - break; - case OPERATION_SET: - MEMCACHED_COMMAND_SET(c->sfd, c->hinfo.key, c->hinfo.nkey, - (ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas); - break; - } + break; + } #endif - switch (ret) { - case ENGINE_SUCCESS: - write_bin_response(c, NULL, 0, 0, 0); - break; - case ENGINE_KEY_EEXISTS: - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, 0); - break; - case ENGINE_KEY_ENOENT: - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_ENOENT, 0); - break; - case ENGINE_NOT_STORED: - /* FIXME: check below code, later. */ - if (c->store_op == OPERATION_ADD) { + switch (ret) { + case ENGINE_SUCCESS: + write_bin_response(c, NULL, 0, 0, 0); + break; + case ENGINE_KEY_EEXISTS: write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, 0); - } else if (c->store_op == OPERATION_REPLACE) { + break; + case ENGINE_KEY_ENOENT: write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_ENOENT, 0); - } else { + break; + case ENGINE_NOT_STORED: + /* FIXME: check below code, later. */ + if (c->store_op == OPERATION_ADD) { + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, 0); + } else if (c->store_op == OPERATION_REPLACE) { + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_ENOENT, 0); + } else { write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_NOT_STORED, 0); + } + break; + case ENGINE_PREFIX_ENAME: + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_PREFIX_ENAME, 0); + break; + case ENGINE_ENOMEM: + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0); + break; + case ENGINE_EINVAL: + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINVAL, 0); + break; + case ENGINE_E2BIG: + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_E2BIG, 0); + break; + case ENGINE_NOT_MY_VBUCKET: + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_NOT_MY_VBUCKET, 0); + break; + case ENGINE_EBADTYPE: + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EBADTYPE, 0); + break; + default: + handle_unexpected_errorcode_bin(c, __func__, ret, 0); } - //write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_NOT_STORED, 0); - break; - case ENGINE_PREFIX_ENAME: - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_PREFIX_ENAME, 0); - break; - case ENGINE_ENOMEM: - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0); - break; - case ENGINE_EINVAL: - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINVAL, 0); - break; - case ENGINE_E2BIG: - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_E2BIG, 0); - break; - case ENGINE_NOT_MY_VBUCKET: - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_NOT_MY_VBUCKET, 0); - break; - case ENGINE_EBADTYPE: - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EBADTYPE, 0); - break; - default: - handle_unexpected_errorcode_bin(c, __func__, ret, 0); } if (c->store_op == OPERATION_CAS) { @@ -3829,7 +3825,7 @@ static void process_bin_get(conn *c) mc_engine.v1->release(mc_engine.v0, c, it); mc_logger->log(EXTENSION_LOG_WARNING, c, "%d: Failed to get item info\n", c->sfd); - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINTERNAL, 0); + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0); break; } @@ -7107,8 +7103,8 @@ static void process_bin_update(conn *c) if (ret == ENGINE_SUCCESS && !mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) { mc_engine.v1->release(mc_engine.v0, c, it); - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINTERNAL, 0); - return; + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0); + ret = ENGINE_ENOMEM; } switch (ret) { @@ -7189,8 +7185,8 @@ static void process_bin_append_prepend(conn *c) if (ret == ENGINE_SUCCESS && !mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) { mc_engine.v1->release(mc_engine.v0, c, it); - write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINTERNAL, 0); - return; + write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0); + ret = ENGINE_ENOMEM; } switch (ret) { @@ -8475,25 +8471,24 @@ static void process_update_command(conn *c, token_t *tokens, const size_t ntoken if (!mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) { mc_engine.v1->release(mc_engine.v0, c, it); out_string(c, "SERVER_ERROR error getting item data"); - ret = ENGINE_FAILED; /* FIXME: error type */ + ret = ENGINE_ENOMEM; /* FIXME: error type */ } else { c->item = it; ritem_set_first(c, CONN_RTYPE_HINFO, vlen); c->store_op = store_op; conn_set_state(c, conn_nread); } - } - if (ret != ENGINE_SUCCESS) { + } else { if (ret == ENGINE_E2BIG) { out_string(c, "CLIENT_ERROR object too large for cache"); } else if (ret == ENGINE_ENOMEM) { out_string(c, "SERVER_ERROR out of memory storing object"); - } else if (ret == ENGINE_FAILED) { - /* out_string() was called above. so, do nothing */ } else { handle_unexpected_errorcode_ascii(c, __func__, ret); } + } + if (ret != ENGINE_SUCCESS) { /* Avoid stale data persisting in cache because we failed alloc. * Unacceptable for SET. Anywhere else too? */