Skip to content

Commit

Permalink
Merge pull request #4763 from cyrusimap/carddav_writecard_x_memory_fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@ksmurchison
ksmurchison authored Dec 7, 2023
2 parents 5af46bc + 5bc0a74 commit 72b106f
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 17 deletions.
45 changes: 45 additions & 0 deletions cassandane/Cassandane/Cyrus/Carddav.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <<EOF;
BEGIN:VCARD
VERSION:4.0
KIND:group
UID:$uid
N:;;;;
FN:My Group
$members
END:VCARD
EOF

$CardDAV->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;
30 changes: 24 additions & 6 deletions imap/carddav_db.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -1420,33 +1418,49 @@ 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;

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: {
Expand All @@ -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;
}

Expand Down Expand Up @@ -1511,6 +1526,8 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb,
default:
break;
}

free(propval);
}

int r;
Expand All @@ -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);

Expand Down
32 changes: 21 additions & 11 deletions imap/http_carddav.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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:
Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)) {
Expand All @@ -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:
Expand Down Expand Up @@ -1527,6 +1535,8 @@ static int carddav_put(struct transaction_t *txn, void *obj,

done:
param_free(&params);
free(uid);
free(fullname);
free(subtype);
free(type);

Expand Down

0 comments on commit 72b106f

Please sign in to comment.