ENGINE-320: done. Change is as follows: All calls to new_bloblist keep the ownership of the blob with the caller; this blob is assigned to bloblist->value_ref, which free_bloblist (and the engine) treat as read_only. Whenever the ENGINE allocates a blob (including via any calls to bloblist_dup()), it uses an internal bloblist creation function which assigns that reference to bloblist->value, which will be deallocated by free_bloblist if present. CALLERS MUST KEEP REFERENCES TO ANY BLOBS PASSED IN, as the engine is allowed to delete the bloblist node for various reason (key removal, etc), even if it cannot touch the blob. ENGINE-320
authorKrista Bennett <krista@pep-project.org>
Thu, 14 Dec 2017 17:05:28 +0100
branchENGINE-320
changeset 23135bd778fe7bf8
parent 2312 b078c7c38b51
child 2314 49be3af38cc3
ENGINE-320: done. Change is as follows: All calls to new_bloblist keep the ownership of the blob with the caller; this blob is assigned to bloblist->value_ref, which free_bloblist (and the engine) treat as read_only. Whenever the ENGINE allocates a blob (including via any calls to bloblist_dup()), it uses an internal bloblist creation function which assigns that reference to bloblist->value, which will be deallocated by free_bloblist if present. CALLERS MUST KEEP REFERENCES TO ANY BLOBS PASSED IN, as the engine is allowed to delete the bloblist node for various reason (key removal, etc), even if it cannot touch the blob.
src/bloblist.c
src/bloblist.h
src/message_api.c
test/bloblist_test.cc
     1.1 --- a/src/bloblist.c	Thu Dec 14 08:51:26 2017 +0100
     1.2 +++ b/src/bloblist.c	Thu Dec 14 17:05:28 2017 +0100
     1.3 @@ -43,6 +43,7 @@
     1.4      return true;
     1.5  }
     1.6  
     1.7 +// Internally, we call new_own_bloblist if we are responsible for blob.
     1.8  DYNAMIC_API bloblist_t *new_bloblist(char *blob, size_t size, const char *mime_type,
     1.9          const char *filename)
    1.10  {
    1.11 @@ -56,16 +57,30 @@
    1.12          bloblist = NULL;
    1.13      }
    1.14  
    1.15 +    bloblist->value = NULL;
    1.16      return bloblist;
    1.17  }
    1.18  
    1.19 +bloblist_t *new_own_bloblist(char *blob, size_t size, const char *mime_type,
    1.20 +        const char *filename)
    1.21 +{
    1.22 +    bloblist_t * bloblist = new_bloblist(blob, size, mime_type, filename);
    1.23 +    if (bloblist)
    1.24 +        bloblist->value = blob;
    1.25 +    return bloblist;
    1.26 +}
    1.27 +
    1.28 +
    1.29  DYNAMIC_API void free_bloblist(bloblist_t *bloblist)
    1.30  {
    1.31      bloblist_t *curr = bloblist;
    1.32  
    1.33      while (curr) {
    1.34          bloblist_t *next = curr->next;
    1.35 -        free(curr->value_ref);
    1.36 +        // value_ref never gets touched. If value exists, it gets
    1.37 +        // cleaned up here. If it doesn't, the mem is owned by the
    1.38 +        // caller. Period.
    1.39 +        free(curr->value);
    1.40          free(curr->mime_type);
    1.41          free(curr->filename);
    1.42          free(curr);
    1.43 @@ -89,7 +104,7 @@
    1.44  
    1.45      memcpy(blob2, src->value_ref, src->size);
    1.46  
    1.47 -    bloblist = new_bloblist(blob2, src->size, src->mime_type, src->filename);
    1.48 +    bloblist = new_own_bloblist(blob2, src->size, src->mime_type, src->filename);
    1.49      if (bloblist == NULL)
    1.50          goto enomem;
    1.51      blob2 = NULL;
    1.52 @@ -106,7 +121,7 @@
    1.53              goto enomem;
    1.54  
    1.55          memcpy(blob2, src_curr->value_ref, src_curr->size);
    1.56 -        *dst_curr_ptr = new_bloblist(blob2, src_curr->size, src_curr->mime_type, src_curr->filename);
    1.57 +        *dst_curr_ptr = new_own_bloblist(blob2, src_curr->size, src_curr->mime_type, src_curr->filename);
    1.58          if (*dst_curr_ptr == NULL)
    1.59              goto enomem;
    1.60  
    1.61 @@ -130,7 +145,7 @@
    1.62          return NULL;
    1.63  
    1.64      if (bloblist == NULL)
    1.65 -        return new_bloblist(blob, size, mime_type, filename);
    1.66 +        return new_own_bloblist(blob, size, mime_type, filename);
    1.67  
    1.68      if (bloblist->value_ref == NULL) { // empty list
    1.69          if (bloblist->next != NULL)
    1.70 @@ -149,7 +164,7 @@
    1.71      while (list_curr->next)
    1.72          list_curr = list_curr->next;
    1.73  
    1.74 -    list_curr->next = new_bloblist(blob, size, mime_type, filename);
    1.75 +    list_curr->next = new_own_bloblist(blob, size, mime_type, filename);
    1.76  
    1.77      assert(list_curr->next);
    1.78      if (list_curr->next == NULL)
     2.1 --- a/src/bloblist.h	Thu Dec 14 08:51:26 2017 +0100
     2.2 +++ b/src/bloblist.h	Thu Dec 14 17:05:28 2017 +0100
     2.3 @@ -17,7 +17,8 @@
     2.4  } content_disposition_type;
     2.5  
     2.6  typedef struct _bloblist_t {
     2.7 -    char *value_ref;                // blob
     2.8 +    char *value_ref;                // read-only reference to blob
     2.9 +    char *value;                    // blob (IF owned by engine)
    2.10      size_t size;                    // size of blob
    2.11      char *mime_type;                // UTF-8 string of MIME type of blob or
    2.12                                      // NULL if unknown
    2.13 @@ -41,8 +42,12 @@
    2.14  //      pointer to new bloblist_t or NULL if out of memory
    2.15  //
    2.16  //  caveat:
    2.17 -//      the ownership of the blob goes to the bloblist; mime_type and filename
    2.18 +//      the ownership of the blob STAYS WITH THE CALLER; mime_type and filename
    2.19  //      are being copied, the originals remain in the ownership of the caller
    2.20 +//      If the engine needs to create a bloblist, it needs to call
    2.21 +//      new_own_bloblist (which is also how bloblist_dup works) so that it
    2.22 +//      can ensure references to memory IT allocates are still available for
    2.23 +//      deallocation
    2.24  
    2.25  DYNAMIC_API bloblist_t *new_bloblist(char *blob, size_t size, const char *mime_type,
    2.26          const char *filename);
    2.27 @@ -52,6 +57,8 @@
    2.28  //
    2.29  //  parameters:
    2.30  //      bloblist (in)   bloblist to free
    2.31 +//
    2.32 +// caveat: does not free any blob which the engine did not allocate
    2.33  
    2.34  DYNAMIC_API void free_bloblist(bloblist_t *bloblist);
    2.35  
    2.36 @@ -113,6 +120,10 @@
    2.37                                                content_disposition_type disposition);
    2.38  
    2.39  
    2.40 +// INTERNAL - only used for bloblist_t objects whose memory the engine OWNS.
    2.41 +bloblist_t *new_own_bloblist(char *blob, size_t size, 
    2.42 +                                const char *mime_type,
    2.43 +                                const char *filename);
    2.44  
    2.45  
    2.46  #ifdef __cplusplus
     3.1 --- a/src/message_api.c	Thu Dec 14 08:51:26 2017 +0100
     3.2 +++ b/src/message_api.c	Thu Dec 14 17:05:28 2017 +0100
     3.3 @@ -901,8 +901,8 @@
     3.4      
     3.5      size_t message_len = strlen(message_text);
     3.6      
     3.7 -    bloblist_t* message_blob = new_bloblist(message_text, message_len,
     3.8 -                                            "message/rfc822", NULL);
     3.9 +    bloblist_t* message_blob = new_own_bloblist(message_text, message_len,
    3.10 +                                                "message/rfc822", NULL);
    3.11      
    3.12      _envelope->attachments = message_blob;
    3.13      if (keep_orig_subject && attachment->shortmsg)
    3.14 @@ -982,7 +982,7 @@
    3.15      if (v == NULL)
    3.16          goto enomem;
    3.17  
    3.18 -    bloblist_t *_a = new_bloblist(v, strlen(v), "application/pgp-encrypted", NULL);
    3.19 +    bloblist_t *_a = new_own_bloblist(v, strlen(v), "application/pgp-encrypted", NULL);
    3.20      if (_a == NULL)
    3.21          goto enomem;
    3.22      dst->attachments = _a;
    3.23 @@ -1232,7 +1232,7 @@
    3.24  static void free_bl_entry(bloblist_t *bl)
    3.25  {
    3.26      if (bl) {
    3.27 -        free(bl->value_ref);
    3.28 +        free(bl->value);
    3.29          free(bl->mime_type);
    3.30          free(bl->filename);
    3.31          free(bl);
    3.32 @@ -2208,7 +2208,7 @@
    3.33  
    3.34      bloblist_t *_m = msg->attachments;
    3.35      if (_m == NULL && src->attachments && src->attachments->value_ref) {
    3.36 -        msg->attachments = new_bloblist(NULL, 0, NULL, NULL);
    3.37 +        msg->attachments = new_own_bloblist(NULL, 0, NULL, NULL);
    3.38          _m = msg->attachments;
    3.39      }
    3.40  
     4.1 --- a/test/bloblist_test.cc	Thu Dec 14 08:51:26 2017 +0100
     4.2 +++ b/test/bloblist_test.cc	Thu Dec 14 17:05:28 2017 +0100
     4.3 @@ -38,7 +38,7 @@
     4.4      assert(val1);
     4.5      assert(val2);
     4.6      assert(val1->size == val2->size);
     4.7 -    assert(test_blob_equals(val1->size, val1->value, val2->size, val2->value));
     4.8 +    assert(test_blob_equals(val1->size, val1->value_ref, val2->size, val2->value_ref));
     4.9      return( ((!val1->mime_type && !val2->mime_type) || (strcmp(val1->mime_type, val2->mime_type) == 0))
    4.10          && ((!val1->filename && !val2->filename) || (strcmp(val1->filename, val2->filename) == 0)));
    4.11  }
    4.12 @@ -49,10 +49,10 @@
    4.13      char* text2 = strdup("More text.");
    4.14      char* text3 = strdup("Unpleasant news and witty one-liners.");
    4.15      char* text4 = strdup("I AM URDNOT WREX AND THIS IS MY PLANET!");
    4.16 -    bloblist_t* bl1 = new_bloblist(text1, strlen(text1) + 1, "text/plain", NULL);
    4.17 -    bloblist_t* bl2 = new_bloblist(text2, strlen(text2) + 1, "text/richtext", "bob.rtf");
    4.18 -    bloblist_t* bl3 = new_bloblist(text3, strlen(text3) + 1, NULL, "dummy.bin");
    4.19 -    bloblist_t* bl4 = new_bloblist(text4, strlen(text4) + 1, NULL, NULL);
    4.20 +    bloblist_t* bl1 = new_own_bloblist(text1, strlen(text1) + 1, "text/plain", NULL);
    4.21 +    bloblist_t* bl2 = new_own_bloblist(text2, strlen(text2) + 1, "text/richtext", "bob.rtf");
    4.22 +    bloblist_t* bl3 = new_own_bloblist(text3, strlen(text3) + 1, NULL, "dummy.bin");
    4.23 +    bloblist_t* bl4 = new_own_bloblist(text4, strlen(text4) + 1, NULL, NULL);
    4.24      
    4.25      bloblist_t* bl_arr[4] = {bl1, bl2, bl3, bl4};
    4.26          
    4.27 @@ -64,7 +64,7 @@
    4.28      assert(new_bl);
    4.29      assert(test_bloblist_node_equals(bl1, new_bl));
    4.30      assert(new_bl->next == NULL);
    4.31 -    assert(bl1->value != new_bl->value);
    4.32 +    assert(bl1->value_ref != new_bl->value_ref);
    4.33      assert(bl1->mime_type != new_bl->mime_type || !(bl1->mime_type || new_bl->mime_type));
    4.34      assert(bl1->filename != new_bl->filename || !(bl1->filename || new_bl->filename));
    4.35      cout << "one-element bloblist duplicated.\n\n";
    4.36 @@ -76,10 +76,10 @@
    4.37      bloblist_t* p;
    4.38      cout << "\ncreating four-element list...\n";
    4.39      bloblist_t* to_copy = bl_arr[0];
    4.40 -    new_bl = bloblist_add(new_bl, strdup(to_copy->value), to_copy->size, to_copy->mime_type, to_copy->filename);
    4.41 +    new_bl = bloblist_add(new_bl, strdup(to_copy->value_ref), to_copy->size, to_copy->mime_type, to_copy->filename);
    4.42      for (i = 1; i < 4; i++) {
    4.43          to_copy = bl_arr[i];
    4.44 -        p = bloblist_add(new_bl, strdup(to_copy->value), to_copy->size, to_copy->mime_type, to_copy->filename);
    4.45 +        p = bloblist_add(new_bl, strdup(to_copy->value_ref), to_copy->size, to_copy->mime_type, to_copy->filename);
    4.46  
    4.47          assert(p);
    4.48      }
    4.49 @@ -90,7 +90,7 @@
    4.50          assert(p);
    4.51          
    4.52          assert(test_bloblist_node_equals(p, bl_arr[i]));
    4.53 -        assert(p->value != bl_arr[i]->value);
    4.54 +        assert(p->value_ref != bl_arr[i]->value_ref);
    4.55          assert(p->mime_type != bl_arr[i]->mime_type || !(p->mime_type || bl_arr[i]->mime_type));
    4.56          assert(p->filename != bl_arr[i]->filename || !(p->filename || bl_arr[i]->filename));
    4.57          
    4.58 @@ -107,7 +107,7 @@
    4.59      while (dup_p) {
    4.60          assert(test_bloblist_node_equals(p, dup_p));
    4.61          assert(p != dup_p);
    4.62 -        assert(p->value != dup_p->value);
    4.63 +        assert(p->value_ref != dup_p->value_ref);
    4.64          assert(p->mime_type != dup_p->mime_type || !(p->mime_type || dup_p->mime_type));
    4.65          assert(p->filename != dup_p->filename || !(p->filename || dup_p->filename));
    4.66  
    4.67 @@ -131,4 +131,3 @@
    4.68      
    4.69      return 0;
    4.70  }
    4.71 -