diff --git a/cassandane/Cassandane/Cyrus/Carddav.pm b/cassandane/Cassandane/Cyrus/Carddav.pm index e47e962ecb..7a67591477 100644 --- a/cassandane/Cassandane/Cyrus/Carddav.pm +++ b/cassandane/Cassandane/Cyrus/Carddav.pm @@ -1422,4 +1422,49 @@ EOF $self->check_replication('cassandane'); } +# If we handle the large number of properties properly, this test will succeed. +# If we overrun the libical ring buffer, this test might fail, +# but it will definitely cause valgrind errors. +sub test_huge_group + :needs_component_httpd +{ + my ($self) = @_; + + my $CardDAV = $self->{carddav}; + + my $members; + + for (1..2500) { + my $ug = Data::UUID->new; + my $uuid = $ug->create_str(); + $members .= "MEMBER:urn:uuid:$_\r\n"; + } + + my $uid = "3b678b69-ca41-461e-b2c7-f96b9fe48d68"; + my $href = "Default/group.ics"; + my $card = <Request('PUT', $href, $card, 'Content-Type' => 'text/vcard'); + my $response = $CardDAV->Request('GET', $href); + my $value = $response->{content}; + $self->assert_matches(qr/$uid/, $value); + + $card =~ s/FN:/NOTE:2500 members\r\nFN:/; + + $CardDAV->Request('PUT', $href, $card, 'Content-Type' => 'text/vcard'); + $response = $CardDAV->Request('GET', $href); + $value = $response->{content}; + $self->assert_matches(qr/$uid/, $value); + $self->assert_matches(qr/2500 members/, $value); +} + 1; diff --git a/imap/carddav_db.c b/imap/carddav_db.c index ccbed7e36b..dbaa5b4b96 100644 --- a/imap/carddav_db.c +++ b/imap/carddav_db.c @@ -1141,9 +1141,7 @@ EXPORTED int carddav_writecard(struct carddav_db *carddavdb, else if (!strcasecmp(name, "nickname")) { if (buf_len(&nicknames)) buf_putc(&nicknames, ','); buf_appendcstr(&nicknames, propval); - cdata->nickname = buf_cstring(&nicknames);; - strarray_appendm(&values, propval); - propval = NULL; + cdata->nickname = buf_cstring(&nicknames); } else if (!strcasecmp(name, "email")) { /* XXX - insert if primary */ @@ -1420,6 +1418,7 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb, int ispinned) { struct buf nicknames = BUF_INITIALIZER; + strarray_t values = STRARRAY_INITIALIZER; strarray_t emails = STRARRAY_INITIALIZER; strarray_t member_uids = STRARRAY_INITIALIZER; vcardproperty *prop; @@ -1427,26 +1426,41 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb, for (prop = vcardcomponent_get_first_property(vcard, VCARD_ANY_PROPERTY); prop; prop = vcardcomponent_get_next_property(vcard, VCARD_ANY_PROPERTY)) { - const char *propval = vcardproperty_get_value_as_string(prop); + /* The libical BUFFER_RING_SIZE used by *_get_value_as_string() + * is 2500 entries. + * A vCard with more than 2500 properties (E.g. a large group card) + * will cause some of our value pointers to be freed out from under us. + * So, we use vcardproperty_get_value_as_string_r() here instead + * and manage the memory ourselves. + */ + char *propval = vcardproperty_get_value_as_string_r(prop); const char *userid = ""; + if (!propval) continue; + switch (vcardproperty_isa(prop)) { case VCARD_UID_PROPERTY: cdata->vcard_uid = propval; + strarray_appendm(&values, propval); + propval = NULL; break; case VCARD_N_PROPERTY: cdata->name = propval; + strarray_appendm(&values, propval); + propval = NULL; break; case VCARD_FN_PROPERTY: cdata->fullname = propval; + strarray_appendm(&values, propval); + propval = NULL; break; case VCARD_NICKNAME_PROPERTY: if (buf_len(&nicknames)) buf_putc(&nicknames, ','); buf_appendcstr(&nicknames, propval); - cdata->nickname = buf_cstring(&nicknames);; + cdata->nickname = buf_cstring(&nicknames); break; case VCARD_EMAIL_PROPERTY: { @@ -1471,8 +1485,9 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb, } } } - strarray_append(&emails, propval); + strarray_appendm(&emails, propval); strarray_append(&emails, ispref ? "1" : ""); + propval = NULL; break; } @@ -1511,6 +1526,8 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb, default: break; } + + free(propval); } int r; @@ -1529,6 +1546,7 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb, if (!r) r = carddav_write_groups(carddavdb, cdata->dav.rowid, &member_uids); buf_free(&nicknames); + strarray_fini(&values); strarray_fini(&emails); strarray_fini(&member_uids); diff --git a/imap/http_carddav.c b/imap/http_carddav.c index 07b3439b53..f3d726c600 100644 --- a/imap/http_carddav.c +++ b/imap/http_carddav.c @@ -602,9 +602,10 @@ static int carddav_store_resource(struct transaction_t *txn, { vcardproperty *prop; struct carddav_data *cdata; - const char *version = NULL, *uid = NULL, *fullname = NULL; + char *version = NULL, *uid = NULL, *fullname = NULL; struct index_record *oldrecord = NULL, record; char *mimehdr; + int r; /* Validate the vCard data */ if (!vcard) { @@ -620,15 +621,15 @@ static int carddav_store_resource(struct transaction_t *txn, switch (vcardproperty_isa(prop)) { case VCARD_VERSION_PROPERTY: - version = propval; + version = xstrdup(propval); break; case VCARD_UID_PROPERTY: - uid = propval; + uid = xstrdup(propval); break; case VCARD_FN_PROPERTY: - fullname = propval; + fullname = xstrdup(propval); break; default: @@ -666,7 +667,8 @@ static int carddav_store_resource(struct transaction_t *txn, if (buf_len(buf) > max_size) { buf_destroy(buf); txn->error.precond = CARDDAV_MAX_SIZE; - return HTTP_FORBIDDEN; + r = HTTP_FORBIDDEN; + goto done; } /* Create and cache RFC 5322 header fields for resource */ @@ -694,10 +696,16 @@ static int carddav_store_resource(struct transaction_t *txn, spool_remove_header(xstrdup("Content-Description"), txn->req_hdrs); /* Store the resource */ - int r = dav_store_resource(txn, buf_cstring(buf), 0, - mailbox, oldrecord, cdata->dav.createdmodseq, - NULL, NULL); + r = dav_store_resource(txn, buf_cstring(buf), 0, + mailbox, oldrecord, cdata->dav.createdmodseq, + NULL, NULL); buf_destroy(buf); + + done: + free(version); + free(uid); + free(fullname); + return r; } @@ -1363,6 +1371,7 @@ static int carddav_put(struct transaction_t *txn, void *obj, { struct carddav_db *db = (struct carddav_db *)destdb; vcardcomponent *vcard = (vcardcomponent *)obj; + char *uid = NULL, *fullname = NULL; char *type = NULL, *subtype = NULL; struct param *params = NULL; const char *want_ver = NULL; @@ -1446,7 +1455,6 @@ static int carddav_put(struct transaction_t *txn, void *obj, /* Sanity check vCard data */ vcardproperty *prop; - const char *uid = NULL, *fullname = NULL; for (prop = vcardcomponent_get_first_property(vcard, VCARD_ANY_PROPERTY); prop; prop = vcardcomponent_get_next_property(vcard, VCARD_ANY_PROPERTY)) { @@ -1469,11 +1477,11 @@ static int carddav_put(struct transaction_t *txn, void *obj, break; case VCARD_UID_PROPERTY: - uid = propval; + uid = xstrdup(propval); break; case VCARD_FN_PROPERTY: - fullname = propval; + fullname = xstrdup(propval); break; default: @@ -1527,6 +1535,8 @@ static int carddav_put(struct transaction_t *txn, void *obj, done: param_free(¶ms); + free(uid); + free(fullname); free(subtype); free(type);