Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buffer: refactor buffer to be dynamically-sized #53

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
105 changes: 72 additions & 33 deletions src/async.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an obscure way of fake-initializing the buffer to allow mpd_async_free() (the mpd_buffer API doesn't document that this is legal). I don't like obscure code like that.
If you refactor this to never fail (like before) and do the first allocation lazily, you can avoid this fragile and cumbersome code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, we would need to do the allocation in mpd_buffer_room(); currently, it returns size_t which would make the OOM case awkward (return (size_t)-1 for instance). We could change that function to write the size in a pointer. Would you be okay with any of these solutions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would mpd_buffer_room() need to do the allocation? It is a simple read-only function which gives information about the buffer state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to initialize in a single place; i realize now that read functions does not use the mpd_buffer_room()
I will update the code to allocate in mpd_buffer_read() and mpd_buffer_write()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated code in commit 154d1a3.
This resulted in an increase of code complexity as the code checks for OOM events in more places.

mpd_async_free(async);
return NULL;
}

if (mpd_buffer_init(&async->output) == NULL) {
mpd_async_free(async);
return NULL;
}

return async;
}
Expand All @@ -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);
}

Expand Down Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you're receiving data from a server which pushes a terabyte to you? DoS bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my understanding, maybe i'm missing something.
I did not change the reading functions to read all received data. It can still fail for large messages. Details below:

mpd_async_read is only used when (i) you are going to read the buffer or (ii) writing/flushing output data. In addition, mpd_buffer_make_room will only increase the size if there wasn't enough space available.

For (i), you will increase the buffer by 256 bytes (if needed) and read data out. If the message was larger than 256 bytes, an error will occur in mpd_async_recv_line. Even if you divide the terabyte message in chunks of 256 bytes, mpd_async_recv_line will continually free buffer area, so more allocations will not happen.

For (ii), the flush/write will stop once output data is no longer available. Output data is generated locally. Assuming you wrote 32MB of output data [1] (13 loops on mpd_sync_send_command_v) and the input buffer was always full, the new input buffer size is only 7424 bytes.

You can increase the input buffer size by calling mpd_async_recv_raw; but this is done deliberately.

[1] MPD will not accept this, but lets say it does for this example.

If you prefer, we could provide in the meson configure the options for the first and limit allocation sizes for the buffer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're wrong. You assume that the message will be delivered in chunks of 256 bytes, but why do you believe that is true?
What if a rogue server sends a single line which is 1 TB long?

Copy link
Contributor Author

@cataldor cataldor Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two cases if you send a single 1 TB line
(i) if you are expecting a line, mpd_async_recv_line will not be able to read the 1 TB. The maximum size it can increase the input buffer is min_size, which is currently 256 bytes. If it does not find \n in the input buffer, an error is set.

(ii) You can ask mpd_async_recv_raw to receive a 1TB. But this is the user choice. I know Linux does not normally respect this, but this would be the ENOMEM case.

Case (ii) is not currently doing this because i forgot to change it. It will only receive up to the current size available. mpc_async_recv_raw() is actually working properly: it reads the available input buffer 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);
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this function do?
Documentation missing!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 14a9563

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;
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use Doxygen-style comments unless you're documenting an API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 14a9563

* 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;

Expand All @@ -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;
}

Expand Down
81 changes: 74 additions & 7 deletions src/buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@
#include <assert.h>
#include <string.h>
#include <stdbool.h>
#include <stdlib.h>

/**
* 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 */
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Badly named variable. This is not the "length" of "data". There is rarely ever that much data in the buffer!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I've renamed to data_size. Fixed in 3cf9285

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that's still a bad name. Your data is not this size. You don't have that much data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we call it data_max_size then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's still ambiguous - reading such a name, I'd think that's the configured limit. What about capacity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed in ef706f9

};

/**
* Initialize an empty buffer.
*/
static inline void
static inline void *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undocumented return value. Why would a caller ever be interested in the value of the allocated buffer pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be changed to bool. The idea is to check if the buffer was able to be allocated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's your idea, then your return value should express exactly that. And should be documented.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why memset()? It wasn't here before. Why is this suddenly necesarry?

Copy link
Contributor Author

@cataldor cataldor Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not necessary; it is just a precaution to initialize the data to a known state
(removed in 5ee2021)


return buffer->data;
}

/**
* Free the buffer area.
*/
static inline void
mpd_buffer_end(struct mpd_buffer *buffer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer _deinit() to match with _init().
Not only does this not match, it can also suggest doing a different thing, e.g. obtain a pointer to the end of the buffer/data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. Fixed in e76558a

{
free(buffer->data);
}

/**
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why memset()?

Copy link
Contributor Author

@cataldor cataldor Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the same reason it is used in mpd_buffer_init: initialize data to a known state
(removed in 5ee2021)

} 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)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove stale (non committed) data from the buffer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to keep the data in a known state.
We always initialize it to zero.
If the user called mpd_buffer_make_room (either directly or indirectly), it may be that it wrote some data and realized that it did not have enough space for it (function quote_vargs); thus, that area is reverted to the zero state. Otherwise, if no non-committed data is present, this memset will do nothing (last parameter is zero).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what's the point of that? Why is "zero state" any more special than "random uncommitted data"? Zero is an arbitrary value just like any other.
I don't like that we discuss this here, in a PR and in a commit that has nothing to do with this. You're sneaking in an unrelated change.

Copy link
Contributor Author

@cataldor cataldor Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I will remove the code related to the known state
(removed in 5ee2021)

}
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
22 changes: 1 addition & 21 deletions src/sync.c
Original file line number Diff line number Diff line change
Expand Up @@ -100,27 +100,7 @@ bool
mpd_sync_send_command_v(struct mpd_async *async, const struct timeval *tv0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: struct timeval is not used here because `mpd_sync_io' is not called anymore;
i have left it since all other functions of mpd_sync receive the timeval as well.
If you prefer that i remove it, let me know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an internal API, we can remove the parameter easily.
But do we want this? Does this function really need dynamic buffer sizes? What is your reason to refactor the output buffer to be dynamic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactoring is to allow libmpdclient to be limited in its message size only by MPD. If i'm right, currently MPD supports up to 8KiB messages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't answer my question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasons were: (i) At least one user would like to send messages > 4KiB (issue #51), and (ii) this code would also adapt in case MPD changes the limit of received messages.
You could also just bump the data size to 8KiB and call it a day (that is possible with that other PR, although, today, the limit is still 4KiB)
Since with this patch we can make room for the message, i don't see the need anymore for mpd_sync_io; so i would remove the tv0 parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#51 does not request being able to send large requests. He complains about the lousy error handling if he tries to send a huge malformed request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the debian bug report:
"Ideally, libmpdclient should support requests of arbitrary size (eventually
reaching the server limit), but if that is not feasible, at least a proper
error reporting would be nice."

I think for the majority of cases 4KiB is more than sufficient; The reason i submitted the other PR is in case the changes i'm proposing here are deemed undesirable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, that's where the Debian bug report deviates from #51, but we were talking about #51.
But even the Debian bug report doesn't explain how such a large request might be useful. Just like #51, the Debian bug report describes a malformed request, and supporting malformed requests is not libmpdclient's goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He also mentions this feature request in #51. The title is about the hard coded limit.

True, the test seems to be about fuzzing, but from the MPD protocol page i didn't find any mention of a limit to the find/search command. In theory, you could concatenate multiple find requests in a single huge line like: ((artist == 'FOO') AND (album == 'BAR')) AND ...

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
Expand Down