Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions buffer/apr_buffer.c
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,9 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_cpy(apr_buffer_t *dst,
apr_size_t size = src->size + src->zero_terminated;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment as above.


void *mem = alloc(ctx, size);
if (!mem) {
return NULL;
}
memcpy(mem, src->d.mem, size);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The documentation says nothing of what happens if alloc() returned NULL (compare with for example apr_buffer_arryadup where it is stated that the function will handle a NULL return from alloc()).
But since memcpy(0, src, size) is undefined behaviour, I think it is a reasonable change. Except, perhaps, if it would be an unacceptable API change to start returning NULL in case of an allocation failure.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've updated the apr_buffer_cpy() documentation in the header to explicitly state that NULL is returned when alloc returns NULL. This makes the API contract clear and consistent with apr_buffer_arraydup() which documents APR_ENOMEM on allocation failure.

A test exercising this path is also included.


dst->zero_terminated = src->zero_terminated;
Expand Down
2 changes: 1 addition & 1 deletion include/apr_buffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ APR_DECLARE(apr_status_t) apr_buffer_dup(apr_buffer_t **out,
* @param src The second buffer
* @param alloc The function callback to allocate memory for the buffer
* @param ctx The context for the callback
* @return Returns dst.
* @return Returns dst, or NULL if alloc is provided and returns NULL.
*/
APR_DECLARE(apr_buffer_t *) apr_buffer_cpy(apr_buffer_t *dst,
const apr_buffer_t *src,
Expand Down
17 changes: 17 additions & 0 deletions test/testbuffer.c
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,22 @@ static void test_compare_buffers(abts_case *tc, void *data)
apr_pool_destroy(pool);
}

static void *test_failing_alloc(void *ctx, apr_size_t size)
{
return NULL;
}

static void test_buffer_cpy_alloc_failure(abts_case *tc, void *data)
{
apr_buffer_t src, dst;

apr_buffer_str_set(&src, "test", APR_BUFFER_STRING);
memset(&dst, 0, sizeof(dst));

ABTS_ASSERT(tc, "apr_buffer_cpy returns NULL on alloc failure",
apr_buffer_cpy(&dst, &src, test_failing_alloc, NULL) == NULL);
}

abts_suite *testbuffer(abts_suite *suite)
{
suite = ADD_SUITE(suite);
Expand All @@ -274,6 +290,7 @@ abts_suite *testbuffer(abts_suite *suite)
abts_run_test(suite, test_null_buffer, NULL);
abts_run_test(suite, test_buffers, NULL);
abts_run_test(suite, test_compare_buffers, NULL);
abts_run_test(suite, test_buffer_cpy_alloc_failure, NULL);

return suite;
}