Re: [PR] fix: five memcpy calls in buffer/apr_buffer in apr_buffer.c [apr]

d-sahlberg commented on code in PR #73:
URL: https://github.com/apache/apr/pull/73#discussion_r3255036075


##########
buffer/apr_buffer.c:
##########
@@ -245,6 +245,9 @@ APR_DECLARE(apr_status_t) apr_buffer_arraydup(apr_buffer_t **out,
     for (i = 0; i < nelts; i++) {
 
         /* absolute value is size of mem buffer including optional terminating zero */
+        if (src->size > APR_BUFFER_MAX) {
+            return APR_EINVAL;
+        }
         apr_size_t size = src->size + src->zero_terminated;

Review Comment:
   Conceptually fine, however it "only" protects the alloc() call from allocating more memory than we say a buffer can hold.
   
   If someone is able to manipulate src->size to something more than what is allocated in src but less than APR_BUFFER_MAX we still have an issue in the memcpy a little later.
   



##########
buffer/apr_buffer.c:
##########
@@ -294,9 +297,15 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_cpy(apr_buffer_t *dst,
     else {
 
         /* absolute value is size of mem buffer including optional terminating zero */
+        if (src->size > APR_BUFFER_MAX) {
+            return NULL;
+        }
         apr_size_t size = src->size + src->zero_terminated;
 
         void *mem = alloc(ctx, size);
+        if (!mem) {
+            return NULL;
+        }
         memcpy(mem, src->d.mem, size);

Review Comment:
   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.
   



##########
buffer/apr_buffer.c:
##########
@@ -294,9 +297,15 @@ APR_DECLARE(apr_buffer_t *) apr_buffer_cpy(apr_buffer_t *dst,
     else {
 
         /* absolute value is size of mem buffer including optional terminating zero */
+        if (src->size > APR_BUFFER_MAX) {
+            return NULL;
+        }
         apr_size_t size = src->size + src->zero_terminated;

Review Comment:
   Same comment as above.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]