ENGINE-246: Fixed read-after-free issue - I'd copied and freed a duplicate message object, thinking that libetpan makes copies, not taking ownership, of message object parts. IN FUTURE, PLEASE NOTE THAT THIS IS WRONG. Leave the message object alone for as long as we're using libetpan, and don't try to optimise around it with copies, because you either lose memory or screw up ownership 2028_fix
authorKrista Bennett <krista@pep-project.org>
Wed, 06 Sep 2017 02:09:14 +0200
branch2028_fix
changeset 203681c4285cd4fc
parent 2035 61b05d325adc
child 2037 148b879873f8
child 2038 a7aa4bf04543
ENGINE-246: Fixed read-after-free issue - I'd copied and freed a duplicate message object, thinking that libetpan makes copies, not taking ownership, of message object parts. IN FUTURE, PLEASE NOTE THAT THIS IS WRONG. Leave the message object alone for as long as we're using libetpan, and don't try to optimise around it with copies, because you either lose memory or screw up ownership
src/mime.c
     1.1 --- a/src/mime.c	Wed Sep 06 01:48:08 2017 +0200
     1.2 +++ b/src/mime.c	Wed Sep 06 02:09:14 2017 +0200
     1.3 @@ -143,7 +143,7 @@
     1.4  static PEP_STATUS mime_html_text(
     1.5          const char *plaintext,
     1.6          const char *htmltext,
     1.7 -        bloblist_t *inlined_attachments,
     1.8 +        bloblist_t *attachments,
     1.9          struct mailmime **result
    1.10      )
    1.11  {
    1.12 @@ -185,6 +185,17 @@
    1.13          submime = NULL;
    1.14      }
    1.15  
    1.16 +    bool inlined_attachments = false;
    1.17 +    
    1.18 +    bloblist_t* traversal_ptr = attachments;
    1.19 +    
    1.20 +    while (traversal_ptr) {
    1.21 +        if (traversal_ptr->disposition == PEP_CONTENT_DISP_INLINE) {
    1.22 +            inlined_attachments = true;
    1.23 +            break;
    1.24 +        }
    1.25 +        traversal_ptr = traversal_ptr->next;
    1.26 +    }
    1.27  
    1.28      if (inlined_attachments) {
    1.29          /* Noooooo... dirk, why do you do this to me? */
    1.30 @@ -229,8 +240,9 @@
    1.31      }
    1.32  
    1.33      bloblist_t *_a;
    1.34 -    for (_a = inlined_attachments; _a != NULL; _a = _a->next) {
    1.35 -
    1.36 +    for (_a = attachments; _a != NULL; _a = _a->next) {
    1.37 +        if (_a->disposition != PEP_CONTENT_DISP_INLINE)
    1.38 +            continue;
    1.39          status = mime_attachment(_a, &submime);
    1.40          if (status != PEP_STATUS_OK)
    1.41              return PEP_UNKNOWN_ERROR; // FIXME
    1.42 @@ -650,33 +662,33 @@
    1.43      return retval;
    1.44  }
    1.45  
    1.46 -static void split_inlined_and_attached(bloblist_t** inlined, bloblist_t** attached) {
    1.47 -    bloblist_t** curr_pp = attached;
    1.48 -    bloblist_t* curr = *curr_pp;
    1.49 -    
    1.50 -    bloblist_t* inline_ret = NULL;
    1.51 -    bloblist_t** inline_curr_pp = &inline_ret;
    1.52 -    
    1.53 -    bloblist_t* att_ret = NULL;
    1.54 -    bloblist_t** att_curr_pp = &att_ret;
    1.55 -    
    1.56 -    while (curr) {
    1.57 -        if (curr->disposition == PEP_CONTENT_DISP_INLINE) {
    1.58 -            *inline_curr_pp = curr;
    1.59 -            inline_curr_pp = &(curr->next);
    1.60 -        }
    1.61 -        else {
    1.62 -            *att_curr_pp = curr;
    1.63 -            att_curr_pp = &(curr->next);            
    1.64 -        }
    1.65 -        *curr_pp = curr->next;
    1.66 -        curr->next = NULL;
    1.67 -        curr = *curr_pp;
    1.68 -    }
    1.69 -    
    1.70 -    *inlined = inline_ret;
    1.71 -    *attached = att_ret;
    1.72 -}
    1.73 +// static void split_inlined_and_attached(bloblist_t** inlined, bloblist_t** attached) {
    1.74 +//     bloblist_t** curr_pp = attached;
    1.75 +//     bloblist_t* curr = *curr_pp;
    1.76 +//     
    1.77 +//     bloblist_t* inline_ret = NULL;
    1.78 +//     bloblist_t** inline_curr_pp = &inline_ret;
    1.79 +//     
    1.80 +//     bloblist_t* att_ret = NULL;
    1.81 +//     bloblist_t** att_curr_pp = &att_ret;
    1.82 +//     
    1.83 +//     while (curr) {
    1.84 +//         if (curr->disposition == PEP_CONTENT_DISP_INLINE) {
    1.85 +//             *inline_curr_pp = curr;
    1.86 +//             inline_curr_pp = &(curr->next);
    1.87 +//         }
    1.88 +//         else {
    1.89 +//             *att_curr_pp = curr;
    1.90 +//             att_curr_pp = &(curr->next);            
    1.91 +//         }
    1.92 +//         *curr_pp = curr->next;
    1.93 +//         curr->next = NULL;
    1.94 +//         curr = *curr_pp;
    1.95 +//     }
    1.96 +//     
    1.97 +//     *inlined = inline_ret;
    1.98 +//     *attached = att_ret;
    1.99 +// }
   1.100  
   1.101  
   1.102  static PEP_STATUS mime_encode_message_plain(
   1.103 @@ -695,17 +707,6 @@
   1.104  
   1.105      assert(msg);
   1.106      assert(result);
   1.107 -
   1.108 -    bloblist_t* inlined_attachments = NULL;
   1.109 -    bloblist_t* normal_attachments = NULL;
   1.110 -
   1.111 -    if (msg->attachments) {
   1.112 -        normal_attachments = bloblist_dup(msg->attachments);
   1.113 -        if (!normal_attachments) {
   1.114 -            status = PEP_OUT_OF_MEMORY;
   1.115 -            goto pep_error;
   1.116 -        }
   1.117 -    }
   1.118      
   1.119      //subject = (msg->shortmsg) ? msg->shortmsg : "pEp";  // not used, yet.
   1.120      plaintext = (msg->longmsg) ? msg->longmsg : "";
   1.121 @@ -714,11 +715,8 @@
   1.122      if (htmltext && (htmltext[0] != '\0')) {
   1.123          /* first, we need to strip out the inlined attachments to ensure this
   1.124             gets set up correctly */
   1.125 -        
   1.126 -        /* Noooooo... dirk, why do you do this to me? */                
   1.127 -        split_inlined_and_attached(&inlined_attachments, &normal_attachments);
   1.128 -
   1.129 -        status = mime_html_text(plaintext, htmltext, inlined_attachments, &mime);
   1.130 +           
   1.131 +        status = mime_html_text(plaintext, htmltext, msg->attachments, &mime);
   1.132                  
   1.133          if (status != PEP_STATUS_OK)
   1.134              goto pep_error;
   1.135 @@ -742,6 +740,18 @@
   1.136              goto enomem;
   1.137      }
   1.138  
   1.139 +    bool normal_attachments = false;
   1.140 +    
   1.141 +    bloblist_t* traversal_ptr = msg->attachments;
   1.142 +    
   1.143 +    while (traversal_ptr) {
   1.144 +        if (traversal_ptr->disposition != PEP_CONTENT_DISP_INLINE) {
   1.145 +            normal_attachments = true;
   1.146 +            break;
   1.147 +        }
   1.148 +        traversal_ptr = traversal_ptr->next;
   1.149 +    }
   1.150 +
   1.151      if (normal_attachments) {
   1.152          submime = mime;
   1.153          mime = part_multiple_new("multipart/mixed");
   1.154 @@ -760,13 +770,11 @@
   1.155          }
   1.156  
   1.157          bloblist_t *_a;
   1.158 -        for (_a = normal_attachments; _a != NULL; _a = _a->next) {
   1.159 +        for (_a = msg->attachments; _a != NULL; _a = _a->next) {
   1.160  
   1.161 -            // FIXME: As far as I can tell, mime_attachment takes ownership of
   1.162 -            // _a->value, so the shell of this bloblist_t is now lost memory.
   1.163 -            // not a problem if we can free the whole message later, but
   1.164 -            // this solution is bad. Need to make a more fundamental message
   1.165 -            // change somewhere, I guess.
   1.166 +            if (_a->disposition == PEP_CONTENT_DISP_INLINE)
   1.167 +                continue;
   1.168 +
   1.169              status = mime_attachment(_a, &submime);
   1.170              if (status != PEP_STATUS_OK)
   1.171                  goto pep_error;
   1.172 @@ -796,12 +804,6 @@
   1.173      if (submime)
   1.174          mailmime_free(submime);
   1.175  
   1.176 -    if (inlined_attachments)
   1.177 -        free_bloblist(inlined_attachments);
   1.178 -
   1.179 -    if (normal_attachments)
   1.180 -        free_bloblist(normal_attachments);
   1.181 -
   1.182      return status;
   1.183  }
   1.184