Skip to content

Commit

Permalink
ADBDEV-2976: Handle multi_perform curl errors immediately (#24)
Browse files Browse the repository at this point in the history
The internal buffer will be filled during the first pxfprotocol_import call.
If there is chunk corruption, we fail here without trying to interpret the
received message during check_response. But if a corrupted chunk is read
during the next filling of the buffer, this chunk will be written in the
internal buffer. Then if trailer CRLF sequence is not faced, CHUNKE_BAD_CHUNK
(with code 3) will be returned and connection must be interrupted as far as
application isn't able to find next chunk start position. But current PXF
implementation doesn't check error buffer in case of internal buffer isn't
empty (we've already placed corrupted chunk here) and tries to interpret it
with gpdbwritableformatter_import. If we are lucky and memory corruption
isn't happened, we receive error during next buffer filling.

To solve this problem, we should check multi_perform result, similar to
curl_easy_perform libcurl function.
  • Loading branch information
RekGRpth authored Nov 18, 2022
1 parent fe6d05a commit 93f41a7
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 10 deletions.
7 changes: 2 additions & 5 deletions external-table/src/libchurl.c
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,6 @@ churl_read_check_connectivity(CHURL_HANDLE handle)
Assert(!context->upload);

fill_internal_buffer(context, 1);
check_response(context);
}

/*
Expand Down Expand Up @@ -626,6 +625,8 @@ multi_perform(churl_context *context)
if (curl_error != CURLM_OK)
elog(ERROR, "internal error: curl_multi_perform failed (%d - %s)",
curl_error, curl_easy_strerror(curl_error));

check_response(context);
}

static bool
Expand Down Expand Up @@ -653,8 +654,6 @@ flush_internal_buffer(churl_context *context)
multi_perform(context);
}

check_response(context);

if ((context->curl_still_running == 0) &&
((context_buffer->top - context_buffer->bot) > 0))
elog(ERROR, "failed sending to remote component %s", get_dest_address(context->curl_handle));
Expand Down Expand Up @@ -709,8 +708,6 @@ finish_upload(churl_context *context)
*/
while (context->curl_still_running != 0)
multi_perform(context);

check_response(context);
}

static void
Expand Down
7 changes: 2 additions & 5 deletions fdw/libchurl.c
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,6 @@ churl_read_check_connectivity(CHURL_HANDLE handle)
Assert(!context->upload);

fill_internal_buffer(context, 1);
check_response(context);
}

/*
Expand Down Expand Up @@ -610,6 +609,8 @@ multi_perform(churl_context *context)
if (curl_error != CURLM_OK)
elog(ERROR, "internal error: curl_multi_perform failed (%d - %s)",
curl_error, curl_easy_strerror(curl_error));

check_response(context);
}

bool
Expand Down Expand Up @@ -641,8 +642,6 @@ flush_internal_buffer(churl_context *context)
((context_buffer->top - context_buffer->bot) > 0))
elog(ERROR, "failed sending to remote component %s", get_dest_address(context->curl_handle));

check_response(context);

context_buffer->top = 0;
context_buffer->bot = 0;
}
Expand Down Expand Up @@ -694,8 +693,6 @@ finish_upload(churl_context *context)
*/
while (context->curl_still_running != 0)
multi_perform(context);

check_response(context);
}

void
Expand Down

0 comments on commit 93f41a7

Please sign in to comment.