Unlock mtxs before calling osql_bplog_free to avoid crash #2013
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5092,135 +5092,104 @@ int osql_comm_send_socksqlreq(char *tohost, const char *sql, int sqlen, | |
| int osql_comm_signal_sqlthr_rc(osql_sess_t *sorese, struct errstat *xerr, | ||
| int rc) | ||
| { | ||
| int msglen = 0; | ||
| char uuid[37]; | ||
| int type; | ||
| union { | ||
| char a [OSQLCOMM_DONE_XERR_UUID_RPL_LEN]; | ||
| char b [OSQLCOMM_DONE_UUID_RPL_LEN]; | ||
| char c [OSQLCOMM_DONE_XERR_RPL_LEN]; | ||
| char d [OSQLCOMM_DONE_RPL_LEN]; | ||
| } largest_message; | ||
| uint8_t *buf = (uint8_t *) &largest_message; | ||
|
|
||
| /* test if the sql thread was the one closing the request, | ||
| * and if so, don't send anything back, request might be gone already anyway | ||
| */ | ||
| if (xerr->errval == SQLITE_ABORT) | ||
| return 0; | ||
|
|
||
| /* if error, lets send the error string */ | ||
| if (!sorese->host) { | ||
| /* local */ | ||
| return osql_chkboard_sqlsession_rc(sorese->rqid, sorese->uuid, | ||
| sorese->nops, NULL, xerr); | ||
| } | ||
|
|
||
| /* remote */ | ||
| if (sorese->rqid == OSQL_RQID_USE_UUID) { | ||
| osql_done_xerr_uuid_t rpl_xerr = {{0}}; | ||
| osql_done_uuid_rpl_t rpl_ok = {{0}}; | ||
|
|
||
| if (rc) { | ||
| msglen = OSQLCOMM_DONE_XERR_UUID_RPL_LEN; | ||
| uint8_t *p_buf = buf; | ||
| uint8_t *p_buf_end = buf + msglen; | ||
| rpl_xerr.hd.type = OSQL_XERR; | ||
| comdb2uuidcpy(rpl_xerr.hd.uuid, sorese->uuid); | ||
| rpl_xerr.dt = *xerr; | ||
|
|
||
| osqlcomm_done_xerr_uuid_type_put(&(rpl_xerr), p_buf, p_buf_end); | ||
|
|
||
|
There was a problem hiding this comment. This was a bug probably, should have been OSQLCOMM_DONE_UUID_RPL_LEN. There was a problem hiding this comment. It most certainly was. I tried to fix it as well (#1868). |
||
| } else { | ||
| msglen = OSQLCOMM_DONE_UUID_RPL_LEN; | ||
| uint8_t *p_buf = buf; | ||
| uint8_t *p_buf_end = buf + msglen; | ||
|
|
||
| rpl_ok.hd.type = OSQL_DONE; | ||
| comdb2uuidcpy(rpl_ok.hd.uuid, sorese->uuid); | ||
| rpl_ok.dt.rc = 0; | ||
| rpl_ok.dt.nops = sorese->nops; | ||
|
|
||
| osqlcomm_done_uuid_rpl_put(&(rpl_ok), p_buf, p_buf_end); | ||
| } | ||
|
|
||
| type = osql_net_type_to_net_uuid_type(NET_OSQL_SIGNAL); | ||
| logmsg(LOGMSG_DEBUG, | ||
| "%s:%d master signaling %s uuid %s with rc=%d xerr=%d\n", | ||
| __func__, __LINE__, sorese->host, | ||
| comdb2uuidstr(sorese->uuid, uuid), rc, xerr->errval); | ||
| } else { | ||
| osql_done_xerr_t rpl_xerr = {{0}}; | ||
| osql_done_rpl_t rpl_ok = {{0}}; | ||
|
|
||
| if (rc) { | ||
| msglen = OSQLCOMM_DONE_XERR_RPL_LEN; | ||
| uint8_t *p_buf = buf; | ||
| uint8_t *p_buf_end = buf + msglen; | ||
| rpl_xerr.hd.type = OSQL_XERR; | ||
| rpl_xerr.hd.sid = sorese->rqid; | ||
| rpl_xerr.dt = *xerr; | ||
|
|
||
| osqlcomm_done_xerr_type_put(&(rpl_xerr), p_buf, p_buf_end); | ||
| } else { | ||
| msglen = OSQLCOMM_DONE_RPL_LEN; | ||
| uint8_t *p_buf = buf; | ||
| uint8_t *p_buf_end = buf + msglen; | ||
|
|
||
| rpl_ok.hd.type = OSQL_DONE; | ||
| rpl_ok.hd.sid = sorese->rqid; | ||
| rpl_ok.dt.rc = 0; | ||
| rpl_ok.dt.nops = sorese->nops; | ||
|
|
||
| osqlcomm_done_rpl_put(&(rpl_ok), p_buf, p_buf_end); | ||
| } | ||
|
|
||
| type = NET_OSQL_SIGNAL; | ||
| logmsg(LOGMSG_DEBUG, | ||
| "%s:%d master signaling %s rqid %llu with rc=%d xerr=%d\n", | ||
| __func__, __LINE__, sorese->host, sorese->rqid, rc, | ||
| xerr->errval); | ||
| } | ||
| #if 0 | ||
| printf("Send %d rqid=%llu tmp=%llu\n", NET_OSQL_SIGNAL, sorese->rqid, osql_log_time()); | ||
| #endif | ||
|
|
||
| /* lazy again, works just because node!=0 */ | ||
| int irc = offload_net_send(sorese->host, type, buf, msglen, 1); | ||
| if (irc) { | ||
| irc = -1; | ||
| logmsg(LOGMSG_ERROR, "%s: error sending done to %s!\n", __func__, | ||
| sorese->host); | ||
| } | ||
|
|
||
| return irc; | ||
|
|
@@ -7297,14 +7266,17 @@ int osql_process_packet(struct ireq *iq, unsigned long long rqid, uuid_t uuid, | |
| static int sorese_rcvreq(char *fromhost, void *dtap, int dtalen, int type, | ||
| int nettype) | ||
| { | ||
| int rc = 0; | ||
| struct ireq *iq = NULL; | ||
| uint8_t *malcd = malloc(OSQL_BP_MAXLEN); | ||
|
There was a problem hiding this comment.
|
||
| if (!malcd) | ||
| goto done; | ||
|
|
||
| osql_sess_t *sess = NULL; | ||
| osql_req_t req; | ||
| bool is_reorder_on = false; | ||
| uint8_t *p_req_buf = dtap; | ||
| const uint8_t *p_req_buf_end = p_req_buf + dtalen; | ||
| uint8_t *p_buf = malcd; | ||
| const uint8_t *p_buf_end = p_buf + OSQL_BP_MAXLEN; | ||
| char *sql; | ||
|
|
@@ -7314,15 +7286,11 @@ static int sorese_rcvreq(char *fromhost, void *dtap, int dtalen, int type, | |
| uuid_t uuid; | ||
| int replaced = 0; | ||
|
|
||
| /* grab the request */ | ||
| if (osql_nettype_is_uuid(nettype)) { | ||
| osql_uuid_req_t uuid_req; | ||
| sql = (char *)osqlcomm_req_uuid_type_get(&uuid_req, p_req_buf, | ||
| p_req_buf_end); | ||
| req.type = uuid_req.type; | ||
| req.rqlen = uuid_req.rqlen; | ||
| req.sqlqlen = uuid_req.sqlqlen; | ||
|
|
@@ -7419,89 +7387,76 @@ static int sorese_rcvreq(char *fromhost, void *dtap, int dtalen, int type, | |
| debug = 1; | ||
| } | ||
|
|
||
| /* Blockproc does not create a copy of the request, | ||
| but creates a different thread to work on it | ||
| Let THAT thread free p_buf (malcd)... */ | ||
|
There was a problem hiding this comment. to answer the above comment, malcd needs to survive...see this line. There was a problem hiding this comment. thanks - we can try passing it in so don't have to get from heap There was a problem hiding this comment. also simplifies cleanup on error There was a problem hiding this comment. good idea, will try in separate PR. |
||
|
|
||
| /* for socksql, is it a retry that needs to be checked for self-deadlock? */ | ||
| if ((type == OSQL_SOCK_REQ || type == OSQL_SOCK_REQ_COST) && | ||
| (req.flags & OSQL_FLAGS_CHECK_SELFLOCK)) { | ||
| /* just make sure we are above the threshold */ | ||
| iq->sorese->verify_retries += gbl_osql_verify_ext_chk; | ||
| } | ||
|
|
||
| done: | ||
| if (rc == 0) { | ||
| /* | ||
| successful, let the session loose | ||
| It is possible that we are clearing sessions due to | ||
| master being rtcpu-ed, and it will wait for the session | ||
| clients to disappear before it will wipe out the session | ||
| */ | ||
|
|
||
| int rc2 = osql_sess_remclient(sess); | ||
| if (rc2) { | ||
| uuidstr_t us; | ||
| comdb2uuidstr(uuid, us); | ||
| logmsg(LOGMSG_ERROR, "%s: failed to release session rqid=[%llx %s] node=%s\n", | ||
| __func__, req.rqid, us, fromhost); | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| if (malcd) | ||
| free(malcd); | ||
|
|
||
| if (iq) | ||
| destroy_ireq(thedb, iq); | ||
|
|
||
| /* notify the sql thread there will be no response! */ | ||
| struct errstat generr = {0}; | ||
|
|
||
| generr.errval = ERR_TRAN_FAILED; | ||
| if (rc == -4) { | ||
| strncpy0(generr.errstr, "fail to create block processor log", | ||
| sizeof(generr.errstr)); | ||
| } else { | ||
| strncpy0(generr.errstr, "failed to create transaction", | ||
| sizeof(generr.errstr)); | ||
| } | ||
|
|
||
| int onstack = 0; | ||
| if (!sess) { | ||
| onstack = 1; /* used to avoid debugging confusion */ | ||
| sess = alloca(sizeof(osql_sess_t)); | ||
| sess->host = fromhost; | ||
| sess->rqid = req.rqid; | ||
| comdb2uuidcpy(sess->uuid, uuid); | ||
| sess->nops = 0; | ||
| } | ||
|
|
||
| int rc2 = osql_comm_signal_sqlthr_rc(sess, &generr, RC_INTERNAL_RETRY); | ||
| if (rc2) { | ||
| uuidstr_t us; | ||
| comdb2uuidstr(uuid, us); | ||
| logmsg(LOGMSG_ERROR, "%s: failed to signaled rqid=[%llx %s] host=%s of " | ||
| "error to create bplog\n", | ||
| __func__, req.rqid, us, fromhost); | ||
| } | ||
| if (onstack) | ||
| sess = NULL; | ||
| else | ||
| osql_close_session(&sess, 0, __func__, NULL, __LINE__); | ||
|
|
||
| return rc; | ||
| } | ||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know not introduced in this PR and noted in prior code-comment about being kludgy - runtime decision for buf size. Perhaps not even a performance issue after optimization. However, while we are here can use a union to clean this up:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea.