From 8932c77fa0c7917db2b64d0b0755c4b960b4ec52 Mon Sep 17 00:00:00 2001 From: Rodrigo Cadore Cataldo Date: Fri, 27 Mar 2020 15:19:37 +0100 Subject: [PATCH 1/9] buffer: refactor buffer to be dynamically-sized Allow the internal buffer of libmpdclient to grow dynamically in size as needed by mpd_{async/sync}_send_command. Previously, the buffer was limited to 4KiB. Address issue reported downstream by Debian[1] and upstream by kaliko (@mxjeff)[2] [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=953110 [2] https://github.com/MusicPlayerDaemon/libmpdclient/issues/51 --- src/async.c | 105 +++++++++++++++++++++++++++++++++++---------------- src/buffer.h | 81 +++++++++++++++++++++++++++++++++++---- src/sync.c | 22 +---------- 3 files changed, 147 insertions(+), 61 deletions(-) diff --git a/src/async.c b/src/async.c index e9912c5d..9a15b57d 100644 --- a/src/async.c +++ b/src/async.c @@ -79,8 +79,16 @@ mpd_async_new(int fd) async->fd = fd; mpd_error_init(&async->error); - mpd_buffer_init(&async->input); - mpd_buffer_init(&async->output); + if (mpd_buffer_init(&async->input) == NULL) { + async->output.data = NULL; + mpd_async_free(async); + return NULL; + } + + if (mpd_buffer_init(&async->output) == NULL) { + mpd_async_free(async); + return NULL; + } return async; } @@ -92,6 +100,8 @@ mpd_async_free(struct mpd_async *async) mpd_socket_close(async->fd); mpd_error_deinit(&async->error); + mpd_buffer_end(&async->input); + mpd_buffer_end(&async->output); free(async); } @@ -179,14 +189,17 @@ mpd_async_read(struct mpd_async *async) { size_t room; ssize_t nbytes; + const size_t min_size = 256; assert(async != NULL); assert(async->fd != MPD_INVALID_SOCKET); assert(!mpd_error_is_defined(&async->error)); + if (!mpd_buffer_make_room(&async->input, min_size)) { + mpd_error_code(&async->error, MPD_ERROR_OOM); + return false; + } room = mpd_buffer_room(&async->input); - if (room == 0) - return true; nbytes = recv(async->fd, mpd_buffer_write(&async->input), room, MSG_DONTWAIT); @@ -276,39 +289,25 @@ mpd_async_io(struct mpd_async *async, enum mpd_async_event events) return true; } -bool -mpd_async_send_command_v(struct mpd_async *async, const char *command, - va_list args) +static bool +quote_vargs(struct mpd_buffer *buffer, char *start, char **end_pos, + va_list args) { - size_t room, length; - char *dest, *end, *p; + char *end, *p; const char *arg; - assert(async != NULL); - assert(command != NULL); - - if (mpd_error_is_defined(&async->error)) - return false; - - room = mpd_buffer_room(&async->output); - length = strlen(command); - if (room <= length) - return false; - - dest = mpd_buffer_write(&async->output); /* -1 because we reserve space for the \n character */ - end = dest + room - 1; - - /* copy the command (no quoting, we asumme it is "clean") */ - - memcpy(dest, command, length); - p = dest + length; + /** + * mpd_async_send_command_v() guarantees that mpd_buffer_room() has at + * least 1 position available (length + 1) + */ + assert(mpd_buffer_room(buffer) >= 1); + end = start + mpd_buffer_room(buffer) - 1; + p = start; /* now append all arguments (quoted) */ - while ((arg = va_arg(args, const char *)) != NULL) { /* append a space separator */ - if (p >= end) return false; @@ -317,17 +316,57 @@ mpd_async_send_command_v(struct mpd_async *async, const char *command, /* quote the argument into the destination buffer */ p = quote(p, end, arg); - assert(p == NULL || (p >= dest && p <= end)); + assert(p == NULL || (p >= start && p <= end)); if (p == NULL) return false; } - /* append the newline to finish this command */ - *p++ = '\n'; + *end_pos = p; + return true; +} + +bool +mpd_async_send_command_v(struct mpd_async *async, const char *command, + va_list args) +{ + char *end_pos = NULL, *write_p; + bool success = false; + size_t length; + va_list cargs; + + assert(async != NULL); + assert(command != NULL); + + if (mpd_error_is_defined(&async->error)) + return false; + + length = strlen(command); + /* we need a '\n' at the end */ + if (!mpd_buffer_make_room(&async->output, length + 1)) { + mpd_error_code(&async->error, MPD_ERROR_OOM); + return false; + } + + /* copy the command (no quoting, we assume it is "clean") */ + memcpy(mpd_buffer_write(&async->output), command, length); + mpd_buffer_expand(&async->output, length); + + while (!success) { + va_copy(cargs, args); + write_p = mpd_buffer_write(&async->output); + success = quote_vargs(&async->output, write_p, &end_pos, cargs); + va_end(cargs); + if (!success) { + if (!mpd_buffer_double_buffer_size(&async->output)) { + mpd_error_code(&async->error, MPD_ERROR_OOM); + return false; + } + } + } - mpd_buffer_expand(&async->output, p - dest); + mpd_buffer_expand(&async->output, end_pos - write_p); return true; } diff --git a/src/buffer.h b/src/buffer.h index cd4a6a5f..93a41de7 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -32,10 +32,10 @@ #include #include #include +#include /** - * A fixed 4kB buffer which can be appended at the end, and consumed - * at the beginning. + * A buffer which can be appended at the end, and consumed at the beginning. */ struct mpd_buffer { /** the next buffer position to write to */ @@ -45,17 +45,37 @@ struct mpd_buffer { unsigned read; /** the actual buffer */ - unsigned char data[4096]; + unsigned char *data; + + /** size of buffer */ + size_t data_len; }; /** * Initialize an empty buffer. */ -static inline void +static inline void * mpd_buffer_init(struct mpd_buffer *buffer) { + const size_t std_data_len = 4096; + buffer->read = 0; buffer->write = 0; + buffer->data_len = std_data_len; + buffer->data = malloc(buffer->data_len); + if (buffer->data != NULL) + memset(buffer->data, 0, buffer->data_len); + + return buffer->data; +} + +/** + * Free the buffer area. + */ +static inline void +mpd_buffer_end(struct mpd_buffer *buffer) +{ + free(buffer->data); } /** @@ -79,12 +99,13 @@ mpd_buffer_move(struct mpd_buffer *buffer) static inline size_t mpd_buffer_room(const struct mpd_buffer *buffer) { - assert(buffer->write <= sizeof(buffer->data)); + assert(buffer->write <= buffer->data_len); assert(buffer->read <= buffer->write); - return sizeof(buffer->data) - (buffer->write - buffer->read); + return buffer->data_len - (buffer->write - buffer->read); } + /** * Checks if the buffer is full, i.e. nothing can be written. */ @@ -125,7 +146,7 @@ mpd_buffer_expand(struct mpd_buffer *buffer, size_t nbytes) static inline size_t mpd_buffer_size(const struct mpd_buffer *buffer) { - assert(buffer->write <= sizeof(buffer->data)); + assert(buffer->write <= buffer->data_len); assert(buffer->read <= buffer->write); return buffer->write - buffer->read; @@ -154,4 +175,50 @@ mpd_buffer_consume(struct mpd_buffer *buffer, size_t nbytes) buffer->read += nbytes; } +/** + * Makes at least min_size bytes available for writing data. + * In other words, mpd_buffer_room() will be >= min_size if called after this + * function. + */ +static inline bool +mpd_buffer_make_room(struct mpd_buffer *buffer, size_t min_avail_len) +{ + size_t newsize; + + if (mpd_buffer_room(buffer) >= min_avail_len) + return true; + + newsize = buffer->data_len * 2; + while (newsize < min_avail_len) + newsize *= 2; + + /* empty buffer */ + if (mpd_buffer_room(buffer) == buffer->data_len) { + free(buffer->data); + buffer->data = malloc(newsize); + if (buffer->data == NULL) + return false; + memset(buffer->data, 0, newsize); + } else { + buffer->data = realloc(buffer->data, newsize); + if (buffer->data == NULL) + return false; + + /* clear region not committed and new region */ + memset(mpd_buffer_write(buffer), 0, + newsize - (buffer->data_len - mpd_buffer_room(buffer))); + } + buffer->data_len = newsize; + return true; +} + +/** + * Double the buffer area. + */ +static inline bool +mpd_buffer_double_buffer_size(struct mpd_buffer *buffer) +{ + return mpd_buffer_make_room(buffer, buffer->data_len * 2); +} + #endif diff --git a/src/sync.c b/src/sync.c index f82d0ec0..4e023c8a 100644 --- a/src/sync.c +++ b/src/sync.c @@ -100,27 +100,7 @@ bool mpd_sync_send_command_v(struct mpd_async *async, const struct timeval *tv0, const char *command, va_list args) { - struct timeval tv, *tvp; - va_list copy; - bool success; - - if (tv0 != NULL) { - tv = *tv0; - tvp = &tv; - } else - tvp = NULL; - - while (true) { - va_copy(copy, args); - success = mpd_async_send_command_v(async, command, copy); - va_end(copy); - - if (success) - return true; - - if (!mpd_sync_io(async, tvp)) - return false; - } + return mpd_async_send_command_v(async, command, args); } bool From 14a9563740b652aaeae007ccc93cadda44211e9c Mon Sep 17 00:00:00 2001 From: Rodrigo Cadore Cataldo Date: Fri, 3 Apr 2020 12:41:58 +0200 Subject: [PATCH 2/9] async: describe the internal function quote_vargs + fix internal comment Describe the functionality of quote_vargs Remove doxygen-style comment from the same function --- src/async.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/async.c b/src/async.c index 9a15b57d..5416500e 100644 --- a/src/async.c +++ b/src/async.c @@ -289,6 +289,14 @@ mpd_async_io(struct mpd_async *async, enum mpd_async_event events) return true; } +/* Insert the argument list args into buffer starting from start up to the + * available buffer space. The inserted arguments are quoted, and special + * characters are properly escaped. Because of these operations, the length to + * be written is only known after the fact. + * + * returns false if the current space available in buffer is insufficient or + * true otherwise. + */ static bool quote_vargs(struct mpd_buffer *buffer, char *start, char **end_pos, va_list args) @@ -297,8 +305,7 @@ quote_vargs(struct mpd_buffer *buffer, char *start, char **end_pos, const char *arg; /* -1 because we reserve space for the \n character */ - /** - * mpd_async_send_command_v() guarantees that mpd_buffer_room() has at + /* mpd_async_send_command_v() guarantees that mpd_buffer_room() has at * least 1 position available (length + 1) */ assert(mpd_buffer_room(buffer) >= 1); From 3cf928540d98463e2cebfa51b42f7a929e63135f Mon Sep 17 00:00:00 2001 From: Rodrigo Cadore Cataldo Date: Fri, 3 Apr 2020 12:51:14 +0200 Subject: [PATCH 3/9] buffer: properly name the buffer size Rename the buffer size to data_size instead of data_len --- src/buffer.h | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/buffer.h b/src/buffer.h index 93a41de7..49be46ba 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -48,7 +48,7 @@ struct mpd_buffer { unsigned char *data; /** size of buffer */ - size_t data_len; + size_t data_size; }; /** @@ -57,14 +57,14 @@ struct mpd_buffer { static inline void * mpd_buffer_init(struct mpd_buffer *buffer) { - const size_t std_data_len = 4096; + const size_t std_data_size = 4096; buffer->read = 0; buffer->write = 0; - buffer->data_len = std_data_len; - buffer->data = malloc(buffer->data_len); + buffer->data_size = std_data_size; + buffer->data = malloc(buffer->data_size); if (buffer->data != NULL) - memset(buffer->data, 0, buffer->data_len); + memset(buffer->data, 0, buffer->data_size); return buffer->data; } @@ -99,10 +99,10 @@ mpd_buffer_move(struct mpd_buffer *buffer) static inline size_t mpd_buffer_room(const struct mpd_buffer *buffer) { - assert(buffer->write <= buffer->data_len); + assert(buffer->write <= buffer->data_size); assert(buffer->read <= buffer->write); - return buffer->data_len - (buffer->write - buffer->read); + return buffer->data_size - (buffer->write - buffer->read); } @@ -146,7 +146,7 @@ mpd_buffer_expand(struct mpd_buffer *buffer, size_t nbytes) static inline size_t mpd_buffer_size(const struct mpd_buffer *buffer) { - assert(buffer->write <= buffer->data_len); + assert(buffer->write <= buffer->data_size); assert(buffer->read <= buffer->write); return buffer->write - buffer->read; @@ -188,12 +188,12 @@ mpd_buffer_make_room(struct mpd_buffer *buffer, size_t min_avail_len) if (mpd_buffer_room(buffer) >= min_avail_len) return true; - newsize = buffer->data_len * 2; + newsize = buffer->data_size * 2; while (newsize < min_avail_len) newsize *= 2; /* empty buffer */ - if (mpd_buffer_room(buffer) == buffer->data_len) { + if (mpd_buffer_room(buffer) == buffer->data_size) { free(buffer->data); buffer->data = malloc(newsize); if (buffer->data == NULL) @@ -206,9 +206,9 @@ mpd_buffer_make_room(struct mpd_buffer *buffer, size_t min_avail_len) /* clear region not committed and new region */ memset(mpd_buffer_write(buffer), 0, - newsize - (buffer->data_len - mpd_buffer_room(buffer))); + newsize - (buffer->data_size - mpd_buffer_room(buffer))); } - buffer->data_len = newsize; + buffer->data_size = newsize; return true; } @@ -218,7 +218,7 @@ mpd_buffer_make_room(struct mpd_buffer *buffer, size_t min_avail_len) static inline bool mpd_buffer_double_buffer_size(struct mpd_buffer *buffer) { - return mpd_buffer_make_room(buffer, buffer->data_len * 2); + return mpd_buffer_make_room(buffer, buffer->data_size * 2); } #endif From e76558a612f6b46325369adceb7429ae8ca8a85f Mon Sep 17 00:00:00 2001 From: Rodrigo Cadore Cataldo Date: Fri, 3 Apr 2020 12:54:30 +0200 Subject: [PATCH 4/9] buffer: properly match deinitialize function Rename mpd_buffer_end() to mpd_buffer_deinit() so that mpd_buffer_init() matches with the latter --- src/buffer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buffer.h b/src/buffer.h index 49be46ba..90dec866 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -73,7 +73,7 @@ mpd_buffer_init(struct mpd_buffer *buffer) * Free the buffer area. */ static inline void -mpd_buffer_end(struct mpd_buffer *buffer) +mpd_buffer_deinit(struct mpd_buffer *buffer) { free(buffer->data); } From a36af8d938d9e4e36c4b365e62e92cb54314f2ad Mon Sep 17 00:00:00 2001 From: Rodrigo Cadore Cataldo Date: Fri, 3 Apr 2020 14:06:58 +0200 Subject: [PATCH 5/9] async: call mpd_buffer_deinit() Change call from mpd_buffer_end() to mpd_buffer_deinit() due to API change --- src/async.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/async.c b/src/async.c index 5416500e..2e058a25 100644 --- a/src/async.c +++ b/src/async.c @@ -100,8 +100,8 @@ mpd_async_free(struct mpd_async *async) mpd_socket_close(async->fd); mpd_error_deinit(&async->error); - mpd_buffer_end(&async->input); - mpd_buffer_end(&async->output); + mpd_buffer_deinit(&async->input); + mpd_buffer_deinit(&async->output); free(async); } From 5ee20212070a9f0772adeacf11e5f20a99e875b6 Mon Sep 17 00:00:00 2001 From: Rodrigo Cadore Cataldo Date: Fri, 3 Apr 2020 14:09:33 +0200 Subject: [PATCH 6/9] buffer: remove memset calls Do not initialize the buffer to zero; data in the buffer is tracked through the buffer API --- src/buffer.h | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/buffer.h b/src/buffer.h index 90dec866..9efcdd45 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -63,8 +63,6 @@ mpd_buffer_init(struct mpd_buffer *buffer) buffer->write = 0; buffer->data_size = std_data_size; buffer->data = malloc(buffer->data_size); - if (buffer->data != NULL) - memset(buffer->data, 0, buffer->data_size); return buffer->data; } @@ -196,20 +194,12 @@ mpd_buffer_make_room(struct mpd_buffer *buffer, size_t min_avail_len) if (mpd_buffer_room(buffer) == buffer->data_size) { free(buffer->data); buffer->data = malloc(newsize); - if (buffer->data == NULL) - return false; - memset(buffer->data, 0, newsize); - } else { + } else buffer->data = realloc(buffer->data, newsize); - if (buffer->data == NULL) - return false; - - /* clear region not committed and new region */ - memset(mpd_buffer_write(buffer), 0, - newsize - (buffer->data_size - mpd_buffer_room(buffer))); - } - buffer->data_size = newsize; - return true; + + if (buffer->data != NULL) + buffer->data_size = newsize; + return buffer->data != NULL; } /** From 154d1a3ef3aab678c129ea14f8951acb1147b063 Mon Sep 17 00:00:00 2001 From: Rodrigo Cadore Cataldo Date: Fri, 3 Apr 2020 15:15:23 +0200 Subject: [PATCH 7/9] buffer: lazy initialize the buffer data Make sure the buffer is only initialized when it is used; in other words, when one of these two functions are called: mpd_buffer_read() or mpd_buffer_write() --- src/async.c | 66 ++++++++++++++++++++++++++++++++++++++-------------- src/buffer.h | 40 ++++++++++++++++++++++++------- 2 files changed, 80 insertions(+), 26 deletions(-) diff --git a/src/async.c b/src/async.c index 2e058a25..9ed03b66 100644 --- a/src/async.c +++ b/src/async.c @@ -78,17 +78,8 @@ mpd_async_new(int fd) async->fd = fd; mpd_error_init(&async->error); - - if (mpd_buffer_init(&async->input) == NULL) { - async->output.data = NULL; - mpd_async_free(async); - return NULL; - } - - if (mpd_buffer_init(&async->output) == NULL) { - mpd_async_free(async); - return NULL; - } + mpd_buffer_init(&async->input); + mpd_buffer_init(&async->output); return async; } @@ -189,6 +180,7 @@ mpd_async_read(struct mpd_async *async) { size_t room; ssize_t nbytes; + char *write_p; const size_t min_size = 256; assert(async != NULL); @@ -197,12 +189,20 @@ mpd_async_read(struct mpd_async *async) if (!mpd_buffer_make_room(&async->input, min_size)) { mpd_error_code(&async->error, MPD_ERROR_OOM); + mpd_error_message(&async->error, + "Out of memory for input buffer"); return false; } room = mpd_buffer_room(&async->input); - nbytes = recv(async->fd, mpd_buffer_write(&async->input), room, - MSG_DONTWAIT); + write_p = mpd_buffer_write(&async->input); + if (write_p == NULL) { + mpd_error_code(&async->error, MPD_ERROR_OOM); + mpd_error_message(&async->error, + "Out of memory for input buffer"); + return false; + } + nbytes = recv(async->fd, write_p, room, MSG_DONTWAIT); if (nbytes < 0) { /* I/O error */ @@ -229,6 +229,7 @@ mpd_async_write(struct mpd_async *async) { size_t size; ssize_t nbytes; + char *dst; assert(async != NULL); assert(async->fd != MPD_INVALID_SOCKET); @@ -238,8 +239,14 @@ mpd_async_write(struct mpd_async *async) if (size == 0) return true; - nbytes = send(async->fd, mpd_buffer_read(&async->output), size, - MSG_DONTWAIT); + dst = mpd_buffer_read(&async->output); + if (dst == NULL) { + mpd_error_code(&async->error, MPD_ERROR_OOM); + mpd_error_message(&async->error, + "Out of memory for output buffer"); + return NULL; + } + nbytes = send(async->fd, dst, size, MSG_DONTWAIT); if (nbytes < 0) { /* I/O error */ @@ -338,6 +345,7 @@ bool mpd_async_send_command_v(struct mpd_async *async, const char *command, va_list args) { + const char *error_msg = "Out of memory for output buffer"; char *end_pos = NULL, *write_p; bool success = false; size_t length; @@ -353,11 +361,18 @@ mpd_async_send_command_v(struct mpd_async *async, const char *command, /* we need a '\n' at the end */ if (!mpd_buffer_make_room(&async->output, length + 1)) { mpd_error_code(&async->error, MPD_ERROR_OOM); + mpd_error_message(&async->error, error_msg); return false; } /* copy the command (no quoting, we assume it is "clean") */ - memcpy(mpd_buffer_write(&async->output), command, length); + write_p = mpd_buffer_write(&async->output); + if (write_p == NULL) { + mpd_error_code(&async->error, MPD_ERROR_OOM); + mpd_error_message(&async->error, error_msg); + return NULL; + } + memcpy(write_p, command, length); mpd_buffer_expand(&async->output, length); while (!success) { @@ -368,6 +383,7 @@ mpd_async_send_command_v(struct mpd_async *async, const char *command, if (!success) { if (!mpd_buffer_double_buffer_size(&async->output)) { mpd_error_code(&async->error, MPD_ERROR_OOM); + mpd_error_message(&async->error, error_msg); return false; } } @@ -406,7 +422,12 @@ mpd_async_recv_line(struct mpd_async *async) return NULL; src = mpd_buffer_read(&async->input); - assert(src != NULL); + if (src == NULL) { + mpd_error_code(&async->error, MPD_ERROR_OOM); + mpd_error_message(&async->error, + "Out of memory for input buffer"); + return NULL; + } newline = memchr(src, '\n', size); if (newline == NULL) { /* line is not finished yet */ @@ -430,14 +451,23 @@ mpd_async_recv_line(struct mpd_async *async) size_t mpd_async_recv_raw(struct mpd_async *async, void *dest, size_t length) { + char *src; size_t max_size = mpd_buffer_size(&async->input); + if (max_size == 0) return 0; if (length > max_size) length = max_size; - memcpy(dest, mpd_buffer_read(&async->input), length); + src = mpd_buffer_read(&async->input); + if (src == NULL) { + mpd_error_code(&async->error, MPD_ERROR_OOM); + mpd_error_message(&async->error, + "Out of memory for input buffer"); + return 0; + } + memcpy(dest, src, length); mpd_buffer_consume(&async->input, length); return length; } diff --git a/src/buffer.h b/src/buffer.h index 9efcdd45..1ac4baa7 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -37,6 +37,7 @@ /** * A buffer which can be appended at the end, and consumed at the beginning. */ +#define DEFAULT_BUFFER_SIZE 4096 struct mpd_buffer { /** the next buffer position to write to */ unsigned write; @@ -47,6 +48,9 @@ struct mpd_buffer { /** the actual buffer */ unsigned char *data; + /** buffer allocated */ + bool data_allocated; + /** size of buffer */ size_t data_size; }; @@ -54,17 +58,14 @@ struct mpd_buffer { /** * Initialize an empty buffer. */ -static inline void * +static inline void mpd_buffer_init(struct mpd_buffer *buffer) { - const size_t std_data_size = 4096; - buffer->read = 0; buffer->write = 0; - buffer->data_size = std_data_size; - buffer->data = malloc(buffer->data_size); - - return buffer->data; + buffer->data = NULL; + buffer->data_allocated = false; + buffer->data_size = DEFAULT_BUFFER_SIZE; } /** @@ -76,6 +77,18 @@ mpd_buffer_deinit(struct mpd_buffer *buffer) free(buffer->data); } +/** + * Allocate the buffer area. + */ +static inline void +mpd_buffer_alloc(struct mpd_buffer *buffer) +{ + assert(!buffer->data_allocated); + + buffer->data = malloc(buffer->data_size); + buffer->data_allocated = true; +} + /** * Move the start of the valid data to the beginning of the allocated * buffer. @@ -83,6 +96,8 @@ mpd_buffer_deinit(struct mpd_buffer *buffer) static inline void mpd_buffer_move(struct mpd_buffer *buffer) { + assert(buffer->data_allocated); + memmove(buffer->data, buffer->data + buffer->read, buffer->write - buffer->read); @@ -122,6 +137,9 @@ mpd_buffer_write(struct mpd_buffer *buffer) { assert(mpd_buffer_room(buffer) > 0); + if (!buffer->data_allocated) + mpd_buffer_alloc(buffer); + mpd_buffer_move(buffer); return buffer->data + buffer->write; } @@ -132,6 +150,7 @@ mpd_buffer_write(struct mpd_buffer *buffer) static inline void mpd_buffer_expand(struct mpd_buffer *buffer, size_t nbytes) { + assert(buffer->data_allocated); assert(mpd_buffer_room(buffer) >= nbytes); buffer->write += nbytes; @@ -159,6 +178,9 @@ mpd_buffer_read(struct mpd_buffer *buffer) { assert(mpd_buffer_size(buffer) > 0); + if (!buffer->data_allocated) + mpd_buffer_alloc(buffer); + return buffer->data + buffer->read; } @@ -168,6 +190,7 @@ mpd_buffer_read(struct mpd_buffer *buffer) static inline void mpd_buffer_consume(struct mpd_buffer *buffer, size_t nbytes) { + assert(buffer->data_allocated); assert(nbytes <= mpd_buffer_size(buffer)); buffer->read += nbytes; @@ -191,7 +214,8 @@ mpd_buffer_make_room(struct mpd_buffer *buffer, size_t min_avail_len) newsize *= 2; /* empty buffer */ - if (mpd_buffer_room(buffer) == buffer->data_size) { + if (!buffer->data_allocated || + mpd_buffer_room(buffer) == buffer->data_size) { free(buffer->data); buffer->data = malloc(newsize); } else From 6e9c70989647ea6dc65f8ee791c3d27b48b1c291 Mon Sep 17 00:00:00 2001 From: Rodrigo Cadore Cataldo Date: Fri, 3 Apr 2020 15:20:24 +0200 Subject: [PATCH 8/9] buffer: missing data_allocated initialization in mpd_buffer_make_room() Make sure data_allocated is initialized when calling malloc() for buffer --- src/buffer.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/buffer.h b/src/buffer.h index 1ac4baa7..98c58f0e 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -218,6 +218,7 @@ mpd_buffer_make_room(struct mpd_buffer *buffer, size_t min_avail_len) mpd_buffer_room(buffer) == buffer->data_size) { free(buffer->data); buffer->data = malloc(newsize); + buffer->data_allocated = true; } else buffer->data = realloc(buffer->data, newsize); From ef706f9e741c8778429b0a99776b8a35625da943 Mon Sep 17 00:00:00 2001 From: Rodrigo Cadore Cataldo Date: Fri, 3 Apr 2020 17:00:11 +0200 Subject: [PATCH 9/9] async: buffer: refactor buffer code to simplify lazy-initialized buffer Simplify the design of the lazy-initialized buffer The buffer may be initialized in three functions: mpd_async_send_command_v(), mpd_async_recv_line(), and mpd_async_recv_raw() --- src/async.c | 84 +++++++++++++++++++++++----------------------------- src/buffer.h | 77 ++++++++++++++++------------------------------- 2 files changed, 63 insertions(+), 98 deletions(-) diff --git a/src/async.c b/src/async.c index 9ed03b66..2417c378 100644 --- a/src/async.c +++ b/src/async.c @@ -55,6 +55,10 @@ typedef SSIZE_T ssize_t; #define MSG_DONTWAIT 0 #endif +/* minimum buffer space available to initialize input data */ +#define ASYNC_BUFFER_INIT_SIZE 4096 +/* minimum buffer space available to receive input data */ +#define ASYNC_BUFFER_MIN_SIZE 256 struct mpd_async { mpd_socket_t fd; @@ -178,31 +182,22 @@ mpd_async_events(const struct mpd_async *async) static bool mpd_async_read(struct mpd_async *async) { - size_t room; ssize_t nbytes; - char *write_p; - const size_t min_size = 256; assert(async != NULL); assert(async->fd != MPD_INVALID_SOCKET); assert(!mpd_error_is_defined(&async->error)); - if (!mpd_buffer_make_room(&async->input, min_size)) { + /* make at least ASYNC_BUFFER_MIN_SIZE available for reading */ + if (!mpd_buffer_make_room(&async->input, ASYNC_BUFFER_MIN_SIZE)) { mpd_error_code(&async->error, MPD_ERROR_OOM); mpd_error_message(&async->error, "Out of memory for input buffer"); return false; } - room = mpd_buffer_room(&async->input); - write_p = mpd_buffer_write(&async->input); - if (write_p == NULL) { - mpd_error_code(&async->error, MPD_ERROR_OOM); - mpd_error_message(&async->error, - "Out of memory for input buffer"); - return false; - } - nbytes = recv(async->fd, write_p, room, MSG_DONTWAIT); + nbytes = recv(async->fd, mpd_buffer_write(&async->input), + mpd_buffer_room(&async->input), MSG_DONTWAIT); if (nbytes < 0) { /* I/O error */ @@ -229,7 +224,6 @@ mpd_async_write(struct mpd_async *async) { size_t size; ssize_t nbytes; - char *dst; assert(async != NULL); assert(async->fd != MPD_INVALID_SOCKET); @@ -239,14 +233,8 @@ mpd_async_write(struct mpd_async *async) if (size == 0) return true; - dst = mpd_buffer_read(&async->output); - if (dst == NULL) { - mpd_error_code(&async->error, MPD_ERROR_OOM); - mpd_error_message(&async->error, - "Out of memory for output buffer"); - return NULL; - } - nbytes = send(async->fd, dst, size, MSG_DONTWAIT); + nbytes = send(async->fd, mpd_buffer_read(&async->output), size, + MSG_DONTWAIT); if (nbytes < 0) { /* I/O error */ @@ -348,7 +336,7 @@ mpd_async_send_command_v(struct mpd_async *async, const char *command, const char *error_msg = "Out of memory for output buffer"; char *end_pos = NULL, *write_p; bool success = false; - size_t length; + size_t length, newcap; va_list cargs; assert(async != NULL); @@ -366,13 +354,7 @@ mpd_async_send_command_v(struct mpd_async *async, const char *command, } /* copy the command (no quoting, we assume it is "clean") */ - write_p = mpd_buffer_write(&async->output); - if (write_p == NULL) { - mpd_error_code(&async->error, MPD_ERROR_OOM); - mpd_error_message(&async->error, error_msg); - return NULL; - } - memcpy(write_p, command, length); + memcpy(mpd_buffer_write(&async->output), command, length); mpd_buffer_expand(&async->output, length); while (!success) { @@ -381,7 +363,8 @@ mpd_async_send_command_v(struct mpd_async *async, const char *command, success = quote_vargs(&async->output, write_p, &end_pos, cargs); va_end(cargs); if (!success) { - if (!mpd_buffer_double_buffer_size(&async->output)) { + newcap = mpd_buffer_capacity(&async->output) * 2; + if (!mpd_buffer_make_room(&async->output, newcap)) { mpd_error_code(&async->error, MPD_ERROR_OOM); mpd_error_message(&async->error, error_msg); return false; @@ -409,6 +392,22 @@ mpd_async_send_command(struct mpd_async *async, const char *command, ...) return success; } +/* Initialize the buffer if necessary + * Set the error code and message in case of error in mpd_buffer_make_room() + */ +static bool +mpd_async_init_buffer(struct mpd_buffer *buffer, + struct mpd_error_info *error) +{ + if (mpd_buffer_capacity(buffer) == 0 && + !mpd_buffer_make_room(buffer, ASYNC_BUFFER_INIT_SIZE)) { + mpd_error_code(error, MPD_ERROR_OOM); + mpd_error_message(error, "Out of memory for buffer"); + return false; + } else + return true; +} + char * mpd_async_recv_line(struct mpd_async *async) { @@ -417,17 +416,14 @@ mpd_async_recv_line(struct mpd_async *async) assert(async != NULL); + if (!mpd_async_init_buffer(&async->input, &async->error)) + return NULL; + size = mpd_buffer_size(&async->input); if (size == 0) return NULL; src = mpd_buffer_read(&async->input); - if (src == NULL) { - mpd_error_code(&async->error, MPD_ERROR_OOM); - mpd_error_message(&async->error, - "Out of memory for input buffer"); - return NULL; - } newline = memchr(src, '\n', size); if (newline == NULL) { /* line is not finished yet */ @@ -438,7 +434,6 @@ mpd_async_recv_line(struct mpd_async *async) mpd_error_message(&async->error, "Response line too large"); } - return NULL; } @@ -451,23 +446,18 @@ mpd_async_recv_line(struct mpd_async *async) size_t mpd_async_recv_raw(struct mpd_async *async, void *dest, size_t length) { - char *src; size_t max_size = mpd_buffer_size(&async->input); + if (!mpd_async_init_buffer(&async->input, &async->error)) + return 0; + if (max_size == 0) return 0; if (length > max_size) length = max_size; - src = mpd_buffer_read(&async->input); - if (src == NULL) { - mpd_error_code(&async->error, MPD_ERROR_OOM); - mpd_error_message(&async->error, - "Out of memory for input buffer"); - return 0; - } - memcpy(dest, src, length); + memcpy(dest, mpd_buffer_read(&async->input), length); mpd_buffer_consume(&async->input, length); return length; } diff --git a/src/buffer.h b/src/buffer.h index 98c58f0e..3d299007 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -37,7 +37,6 @@ /** * A buffer which can be appended at the end, and consumed at the beginning. */ -#define DEFAULT_BUFFER_SIZE 4096 struct mpd_buffer { /** the next buffer position to write to */ unsigned write; @@ -48,11 +47,8 @@ struct mpd_buffer { /** the actual buffer */ unsigned char *data; - /** buffer allocated */ - bool data_allocated; - - /** size of buffer */ - size_t data_size; + /** capacity of buffer */ + size_t capacity; }; /** @@ -64,8 +60,7 @@ mpd_buffer_init(struct mpd_buffer *buffer) buffer->read = 0; buffer->write = 0; buffer->data = NULL; - buffer->data_allocated = false; - buffer->data_size = DEFAULT_BUFFER_SIZE; + buffer->capacity = 0; } /** @@ -78,15 +73,12 @@ mpd_buffer_deinit(struct mpd_buffer *buffer) } /** - * Allocate the buffer area. + * Return the capacity of the buffer. */ -static inline void -mpd_buffer_alloc(struct mpd_buffer *buffer) +static inline size_t +mpd_buffer_capacity(struct mpd_buffer *buffer) { - assert(!buffer->data_allocated); - - buffer->data = malloc(buffer->data_size); - buffer->data_allocated = true; + return buffer->capacity; } /** @@ -96,8 +88,6 @@ mpd_buffer_alloc(struct mpd_buffer *buffer) static inline void mpd_buffer_move(struct mpd_buffer *buffer) { - assert(buffer->data_allocated); - memmove(buffer->data, buffer->data + buffer->read, buffer->write - buffer->read); @@ -112,10 +102,10 @@ mpd_buffer_move(struct mpd_buffer *buffer) static inline size_t mpd_buffer_room(const struct mpd_buffer *buffer) { - assert(buffer->write <= buffer->data_size); + assert(buffer->write <= buffer->capacity); assert(buffer->read <= buffer->write); - return buffer->data_size - (buffer->write - buffer->read); + return buffer->capacity - (buffer->write - buffer->read); } @@ -137,9 +127,6 @@ mpd_buffer_write(struct mpd_buffer *buffer) { assert(mpd_buffer_room(buffer) > 0); - if (!buffer->data_allocated) - mpd_buffer_alloc(buffer); - mpd_buffer_move(buffer); return buffer->data + buffer->write; } @@ -150,7 +137,6 @@ mpd_buffer_write(struct mpd_buffer *buffer) static inline void mpd_buffer_expand(struct mpd_buffer *buffer, size_t nbytes) { - assert(buffer->data_allocated); assert(mpd_buffer_room(buffer) >= nbytes); buffer->write += nbytes; @@ -163,7 +149,7 @@ mpd_buffer_expand(struct mpd_buffer *buffer, size_t nbytes) static inline size_t mpd_buffer_size(const struct mpd_buffer *buffer) { - assert(buffer->write <= buffer->data_size); + assert(buffer->write <= buffer->capacity); assert(buffer->read <= buffer->write); return buffer->write - buffer->read; @@ -178,9 +164,6 @@ mpd_buffer_read(struct mpd_buffer *buffer) { assert(mpd_buffer_size(buffer) > 0); - if (!buffer->data_allocated) - mpd_buffer_alloc(buffer); - return buffer->data + buffer->read; } @@ -190,50 +173,42 @@ mpd_buffer_read(struct mpd_buffer *buffer) static inline void mpd_buffer_consume(struct mpd_buffer *buffer, size_t nbytes) { - assert(buffer->data_allocated); assert(nbytes <= mpd_buffer_size(buffer)); buffer->read += nbytes; } /** - * Makes at least min_size bytes available for writing data. - * In other words, mpd_buffer_room() will be >= min_size if called after this - * function. + * Makes at least min_avail_len bytes available for reading/writing data. + * In other words, mpd_buffer_room() will be >= min_avail_len if called after + * this function. */ static inline bool mpd_buffer_make_room(struct mpd_buffer *buffer, size_t min_avail_len) { - size_t newsize; + size_t newcap; if (mpd_buffer_room(buffer) >= min_avail_len) return true; - newsize = buffer->data_size * 2; - while (newsize < min_avail_len) - newsize *= 2; + if (mpd_buffer_capacity(buffer) == 0) + newcap = min_avail_len; + else { + newcap = buffer->capacity * 2; + while (newcap < min_avail_len) + newcap *= 2; + } /* empty buffer */ - if (!buffer->data_allocated || - mpd_buffer_room(buffer) == buffer->data_size) { - free(buffer->data); - buffer->data = malloc(newsize); - buffer->data_allocated = true; + if (mpd_buffer_room(buffer) == mpd_buffer_capacity(buffer)) { + mpd_buffer_deinit(buffer); + buffer->data = malloc(newcap); } else - buffer->data = realloc(buffer->data, newsize); + buffer->data = realloc(buffer->data, newcap); if (buffer->data != NULL) - buffer->data_size = newsize; + buffer->capacity = newcap; return buffer->data != NULL; } -/** - * Double the buffer area. - */ -static inline bool -mpd_buffer_double_buffer_size(struct mpd_buffer *buffer) -{ - return mpd_buffer_make_room(buffer, buffer->data_size * 2); -} - #endif