Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upftplistparser: free off temporary memory always #2013
Conversation
When using the FTP list parser, ensure that the memory that's allocated is always freed. Detected by OSS-fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=3682
This comment has been minimized.
This comment has been minimized.
|
Lovely. Thanks a lot! I would prefer to have the label in lowercase like wo do elsewhere, and while there I think we should cleanup the From a8ea21e4f67e2d6065ed3ea276e9e843212017a2 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Wed, 25 Oct 2017 18:19:44 +0200
Subject: [PATCH] ftplistparser: follow-up cleanup to remove PL_ERROR()
---
lib/ftplistparser.c | 167 ++++++++++++++++++++++++----------------------------
1 file changed, 78 insertions(+), 89 deletions(-)
diff --git a/lib/ftplistparser.c b/lib/ftplistparser.c
index 58a49722b..262ac0306 100644
--- a/lib/ftplistparser.c
+++ b/lib/ftplistparser.c
@@ -262,20 +262,10 @@ static int ftp_pl_get_permission(const char *str)
permissions |= FTP_LP_MALFORMATED_PERM;
return permissions;
}
-static void PL_ERROR(struct connectdata *conn, CURLcode err)
-{
- struct ftp_wc_tmpdata *tmpdata = conn->data->wildcard.tmp;
- struct ftp_parselist_data *parser = tmpdata->parser;
- if(parser->file_data)
- Curl_fileinfo_dtor(NULL, parser->file_data);
- parser->file_data = NULL;
- parser->error = err;
-}
-
static CURLcode ftp_pl_insert_finfo(struct connectdata *conn,
struct fileinfo *infop)
{
curl_fnmatch_callback compare;
struct WildcardData *wc = &conn->data->wildcard;
@@ -345,11 +335,11 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
* 1. call => OK..
* 2. call => OUT_OF_MEMORY (or other error)
* 3. (last) call => is skipped RIGHT HERE and the error is hadled later
* in wc_statemach()
*/
- goto EXIT_LABEL;
+ goto fail;
}
if(parser->os_type == OS_TYPE_UNKNOWN && bufflen > 0) {
/* considering info about FILE response format */
parser->os_type = (buffer[0] >= '0' && buffer[0] <= '9') ?
@@ -361,16 +351,16 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
char c = buffer[i];
if(!parser->file_data) { /* tmp file data is not allocated yet */
parser->file_data = Curl_fileinfo_alloc();
if(!parser->file_data) {
parser->error = CURLE_OUT_OF_MEMORY;
- goto EXIT_LABEL;
+ goto fail;
}
parser->file_data->info.b_data = malloc(FTP_BUFFER_ALLOCSIZE);
if(!parser->file_data->info.b_data) {
- PL_ERROR(conn, CURLE_OUT_OF_MEMORY);
- goto EXIT_LABEL;
+ parser->error = CURLE_OUT_OF_MEMORY;
+ goto fail;
}
parser->file_data->info.b_size = FTP_BUFFER_ALLOCSIZE;
parser->item_offset = 0;
parser->item_length = 0;
}
@@ -389,12 +379,11 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
}
else {
Curl_fileinfo_dtor(NULL, parser->file_data);
parser->file_data = NULL;
parser->error = CURLE_OUT_OF_MEMORY;
- PL_ERROR(conn, CURLE_OUT_OF_MEMORY);
- goto EXIT_LABEL;
+ goto fail;
}
}
switch(parser->os_type) {
case OS_TYPE_UNIX:
@@ -428,19 +417,19 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
while(ISSPACE(*endptr))
endptr++;
while(ISDIGIT(*endptr))
endptr++;
if(*endptr != 0) {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
parser->state.UNIX.main = PL_UNIX_FILETYPE;
finfo->b_used = 0;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
}
break;
}
break;
@@ -469,36 +458,36 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
break;
case 'D':
finfo->filetype = CURLFILETYPE_DOOR;
break;
default:
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
parser->state.UNIX.main = PL_UNIX_PERMISSION;
parser->item_length = 0;
parser->item_offset = 1;
break;
case PL_UNIX_PERMISSION:
parser->item_length++;
if(parser->item_length <= 9) {
if(!strchr("rwx-tTsS", c)) {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
}
else if(parser->item_length == 10) {
unsigned int perm;
if(c != ' ') {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
finfo->b_data[10] = 0; /* terminate permissions */
perm = ftp_pl_get_permission(finfo->b_data + parser->item_offset);
if(perm & FTP_LP_MALFORMATED_PERM) {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
parser->file_data->info.flags |= CURLFINFOFLAG_KNOWN_PERM;
parser->file_data->info.perm = perm;
parser->offsets.perm = parser->item_offset;
@@ -515,12 +504,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->item_offset = finfo->b_used - 1;
parser->item_length = 1;
parser->state.UNIX.sub.hlinks = PL_UNIX_HLINKS_NUMBER;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
}
break;
case PL_UNIX_HLINKS_NUMBER:
parser->item_length ++;
@@ -537,12 +526,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->item_offset = 0;
parser->state.UNIX.main = PL_UNIX_USER;
parser->state.UNIX.sub.user = PL_UNIX_USER_PRESPACE;
}
else if(c < '0' || c > '9') {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
}
break;
case PL_UNIX_USER:
@@ -597,12 +586,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->item_offset = finfo->b_used - 1;
parser->item_length = 1;
parser->state.UNIX.sub.size = PL_UNIX_SIZE_NUMBER;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
}
break;
case PL_UNIX_SIZE_NUMBER:
parser->item_length++;
@@ -622,12 +611,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->state.UNIX.main = PL_UNIX_TIME;
parser->state.UNIX.sub.time = PL_UNIX_TIME_PREPART1;
}
}
else if(!ISDIGIT(c)) {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
}
break;
case PL_UNIX_TIME:
@@ -638,56 +627,56 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->item_offset = finfo->b_used -1;
parser->item_length = 1;
parser->state.UNIX.sub.time = PL_UNIX_TIME_PART1;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
}
break;
case PL_UNIX_TIME_PART1:
parser->item_length++;
if(c == ' ') {
parser->state.UNIX.sub.time = PL_UNIX_TIME_PREPART2;
}
else if(!ISALNUM(c) && c != '.') {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
case PL_UNIX_TIME_PREPART2:
parser->item_length++;
if(c != ' ') {
if(ISALNUM(c)) {
parser->state.UNIX.sub.time = PL_UNIX_TIME_PART2;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
}
break;
case PL_UNIX_TIME_PART2:
parser->item_length++;
if(c == ' ') {
parser->state.UNIX.sub.time = PL_UNIX_TIME_PREPART3;
}
else if(!ISALNUM(c) && c != '.') {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
case PL_UNIX_TIME_PREPART3:
parser->item_length++;
if(c != ' ') {
if(ISALNUM(c)) {
parser->state.UNIX.sub.time = PL_UNIX_TIME_PART3;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
}
break;
case PL_UNIX_TIME_PART3:
parser->item_length++;
@@ -707,12 +696,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->state.UNIX.main = PL_UNIX_FILENAME;
parser->state.UNIX.sub.filename = PL_UNIX_FILENAME_PRESPACE;
}
}
else if(!ISALNUM(c) && c != '.' && c != ':') {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
}
break;
case PL_UNIX_FILENAME:
@@ -733,29 +722,29 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
finfo->b_data[parser->item_offset + parser->item_length - 1] = 0;
parser->offsets.filename = parser->item_offset;
parser->state.UNIX.main = PL_UNIX_FILETYPE;
result = ftp_pl_insert_finfo(conn, infop);
if(result) {
- PL_ERROR(conn, result);
- goto EXIT_LABEL;
+ parser->error = result;
+ goto fail;
}
}
break;
case PL_UNIX_FILENAME_WINDOWSEOL:
if(c == '\n') {
finfo->b_data[parser->item_offset + parser->item_length - 1] = 0;
parser->offsets.filename = parser->item_offset;
parser->state.UNIX.main = PL_UNIX_FILETYPE;
result = ftp_pl_insert_finfo(conn, infop);
if(result) {
- PL_ERROR(conn, result);
- goto EXIT_LABEL;
+ parser->error = result;
+ goto fail;
}
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
}
break;
case PL_UNIX_SYMLINK:
@@ -771,22 +760,22 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->item_length++;
if(c == ' ') {
parser->state.UNIX.sub.symlink = PL_UNIX_SYMLINK_PRETARGET1;
}
else if(c == '\r' || c == '\n') {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
case PL_UNIX_SYMLINK_PRETARGET1:
parser->item_length++;
if(c == '-') {
parser->state.UNIX.sub.symlink = PL_UNIX_SYMLINK_PRETARGET2;
}
else if(c == '\r' || c == '\n') {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
else {
parser->state.UNIX.sub.symlink = PL_UNIX_SYMLINK_NAME;
}
break;
@@ -794,12 +783,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->item_length++;
if(c == '>') {
parser->state.UNIX.sub.symlink = PL_UNIX_SYMLINK_PRETARGET3;
}
else if(c == '\r' || c == '\n') {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
else {
parser->state.UNIX.sub.symlink = PL_UNIX_SYMLINK_NAME;
}
break;
@@ -812,12 +801,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->offsets.filename = parser->item_offset;
parser->item_length = 0;
parser->item_offset = 0;
}
else if(c == '\r' || c == '\n') {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
else {
parser->state.UNIX.sub.symlink = PL_UNIX_SYMLINK_NAME;
}
break;
@@ -826,12 +815,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->state.UNIX.sub.symlink = PL_UNIX_SYMLINK_TARGET;
parser->item_offset = finfo->b_used - 1;
parser->item_length = 1;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
case PL_UNIX_SYMLINK_TARGET:
parser->item_length++;
if(c == '\r') {
@@ -840,30 +829,30 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
else if(c == '\n') {
finfo->b_data[parser->item_offset + parser->item_length - 1] = 0;
parser->offsets.symlink_target = parser->item_offset;
result = ftp_pl_insert_finfo(conn, infop);
if(result) {
- PL_ERROR(conn, result);
- goto EXIT_LABEL;
+ parser->error = result;
+ goto fail;
}
parser->state.UNIX.main = PL_UNIX_FILETYPE;
}
break;
case PL_UNIX_SYMLINK_WINDOWSEOL:
if(c == '\n') {
finfo->b_data[parser->item_offset + parser->item_length - 1] = 0;
parser->offsets.symlink_target = parser->item_offset;
result = ftp_pl_insert_finfo(conn, infop);
if(result) {
- PL_ERROR(conn, result);
- goto EXIT_LABEL;
+ parser->error = result;
+ goto fail;
}
parser->state.UNIX.main = PL_UNIX_FILETYPE;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
}
break;
}
@@ -872,27 +861,27 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
switch(parser->state.NT.main) {
case PL_WINNT_DATE:
parser->item_length++;
if(parser->item_length < 9) {
if(!strchr("0123456789-", c)) { /* only simple control */
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
}
else if(parser->item_length == 9) {
if(c == ' ') {
parser->state.NT.main = PL_WINNT_TIME;
parser->state.NT.sub.time = PL_WINNT_TIME_PRESPACE;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
case PL_WINNT_TIME:
parser->item_length++;
switch(parser->state.NT.sub.time) {
@@ -908,12 +897,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->state.NT.main = PL_WINNT_DIRORSIZE;
parser->state.NT.sub.dirorsize = PL_WINNT_DIRORSIZE_PRESPACE;
parser->item_length = 0;
}
else if(!strchr("APM0123456789:", c)) {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
}
break;
case PL_WINNT_DIRORSIZE:
@@ -939,12 +928,12 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
else {
char *endptr;
if(curlx_strtoofft(finfo->b_data +
parser->item_offset,
&endptr, 10, &finfo->size)) {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
/* correct file type */
parser->file_data->info.filetype = CURLFILETYPE_FILE;
}
@@ -975,49 +964,49 @@ size_t Curl_ftp_parselist(char *buffer, size_t size, size_t nmemb,
parser->offsets.filename = parser->item_offset;
finfo->b_data[finfo->b_used - 1] = 0;
parser->offsets.filename = parser->item_offset;
result = ftp_pl_insert_finfo(conn, infop);
if(result) {
- PL_ERROR(conn, result);
- goto EXIT_LABEL;
+ parser->error = result;
+ goto fail;
}
parser->state.NT.main = PL_WINNT_DATE;
parser->state.NT.sub.filename = PL_WINNT_FILENAME_PRESPACE;
}
break;
case PL_WINNT_FILENAME_WINEOL:
if(c == '\n') {
parser->offsets.filename = parser->item_offset;
result = ftp_pl_insert_finfo(conn, infop);
if(result) {
- PL_ERROR(conn, result);
- goto EXIT_LABEL;
+ parser->error = result;
+ goto fail;
}
parser->state.NT.main = PL_WINNT_DATE;
parser->state.NT.sub.filename = PL_WINNT_FILENAME_PRESPACE;
}
else {
- PL_ERROR(conn, CURLE_FTP_BAD_FILE_LIST);
- goto EXIT_LABEL;
+ parser->error = CURLE_FTP_BAD_FILE_LIST;
+ goto fail;
}
break;
}
break;
}
break;
default:
retsize = bufflen + 1;
- goto EXIT_LABEL;
+ goto fail;
}
i++;
}
-EXIT_LABEL:
+fail:
/* Clean up any allocated memory. */
- if(parser->file_data != NULL) {
+ if(parser->file_data) {
Curl_fileinfo_dtor(NULL, parser->file_data);
parser->file_data = NULL;
}
return retsize;
--
2.14.1
|
This comment has been minimized.
This comment has been minimized.
|
Looks great. Do you want me to patch it on top of mine, or do you want to handle that as part of merging in? |
This comment has been minimized.
This comment has been minimized.
|
I just wanted to run it by you first, I'll just add that commit when I merge. |
This comment has been minimized.
This comment has been minimized.
|
Thanks! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
cmeister2 commentedOct 25, 2017
When using the FTP list parser, ensure that the memory that's
allocated is always freed.
Detected by OSS-fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=3682
I haven't been able to test this using the standard test suite, but the fuzzer no longer complains about leaking memory after applying this patch.