Skip to content

Commit 72b106f

Browse files
ksmurchisonksmurchison
authored
Merge pull request #4763 from cyrusimap/carddav_writecard_x_memory_fix
2 parents 5af46bc + 5bc0a74 commit 72b106f

File tree

3 files changed

+90
-17
lines changed

3 files changed

+90
-17
lines changed

Diff for: ‎cassandane/Cassandane/Cyrus/Carddav.pm

+45
Original file line numberDiff line numberDiff line change
@@ -1422,4 +1422,49 @@ EOF
14221422
$self->check_replication('cassandane');
14231423
}
14241424

1425+
# If we handle the large number of properties properly, this test will succeed.
1426+
# If we overrun the libical ring buffer, this test might fail,
1427+
# but it will definitely cause valgrind errors.
1428+
sub test_huge_group
1429+
:needs_component_httpd
1430+
{
1431+
my ($self) = @_;
1432+
1433+
my $CardDAV = $self->{carddav};
1434+
1435+
my $members;
1436+
1437+
for (1..2500) {
1438+
my $ug = Data::UUID->new;
1439+
my $uuid = $ug->create_str();
1440+
$members .= "MEMBER:urn:uuid:$_\r\n";
1441+
}
1442+
1443+
my $uid = "3b678b69-ca41-461e-b2c7-f96b9fe48d68";
1444+
my $href = "Default/group.ics";
1445+
my $card = <<EOF;
1446+
BEGIN:VCARD
1447+
VERSION:4.0
1448+
KIND:group
1449+
UID:$uid
1450+
N:;;;;
1451+
FN:My Group
1452+
$members
1453+
END:VCARD
1454+
EOF
1455+
1456+
$CardDAV->Request('PUT', $href, $card, 'Content-Type' => 'text/vcard');
1457+
my $response = $CardDAV->Request('GET', $href);
1458+
my $value = $response->{content};
1459+
$self->assert_matches(qr/$uid/, $value);
1460+
1461+
$card =~ s/FN:/NOTE:2500 members\r\nFN:/;
1462+
1463+
$CardDAV->Request('PUT', $href, $card, 'Content-Type' => 'text/vcard');
1464+
$response = $CardDAV->Request('GET', $href);
1465+
$value = $response->{content};
1466+
$self->assert_matches(qr/$uid/, $value);
1467+
$self->assert_matches(qr/2500 members/, $value);
1468+
}
1469+
14251470
1;

Diff for: ‎imap/carddav_db.c

+24-6
Original file line numberDiff line numberDiff line change
@@ -1141,9 +1141,7 @@ EXPORTED int carddav_writecard(struct carddav_db *carddavdb,
11411141
else if (!strcasecmp(name, "nickname")) {
11421142
if (buf_len(&nicknames)) buf_putc(&nicknames, ',');
11431143
buf_appendcstr(&nicknames, propval);
1144-
cdata->nickname = buf_cstring(&nicknames);;
1145-
strarray_appendm(&values, propval);
1146-
propval = NULL;
1144+
cdata->nickname = buf_cstring(&nicknames);
11471145
}
11481146
else if (!strcasecmp(name, "email")) {
11491147
/* XXX - insert if primary */
@@ -1420,33 +1418,49 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb,
14201418
int ispinned)
14211419
{
14221420
struct buf nicknames = BUF_INITIALIZER;
1421+
strarray_t values = STRARRAY_INITIALIZER;
14231422
strarray_t emails = STRARRAY_INITIALIZER;
14241423
strarray_t member_uids = STRARRAY_INITIALIZER;
14251424
vcardproperty *prop;
14261425

14271426
for (prop = vcardcomponent_get_first_property(vcard, VCARD_ANY_PROPERTY);
14281427
prop;
14291428
prop = vcardcomponent_get_next_property(vcard, VCARD_ANY_PROPERTY)) {
1430-
const char *propval = vcardproperty_get_value_as_string(prop);
1429+
/* The libical BUFFER_RING_SIZE used by *_get_value_as_string()
1430+
* is 2500 entries.
1431+
* A vCard with more than 2500 properties (E.g. a large group card)
1432+
* will cause some of our value pointers to be freed out from under us.
1433+
* So, we use vcardproperty_get_value_as_string_r() here instead
1434+
* and manage the memory ourselves.
1435+
*/
1436+
char *propval = vcardproperty_get_value_as_string_r(prop);
14311437
const char *userid = "";
14321438

1439+
if (!propval) continue;
1440+
14331441
switch (vcardproperty_isa(prop)) {
14341442
case VCARD_UID_PROPERTY:
14351443
cdata->vcard_uid = propval;
1444+
strarray_appendm(&values, propval);
1445+
propval = NULL;
14361446
break;
14371447

14381448
case VCARD_N_PROPERTY:
14391449
cdata->name = propval;
1450+
strarray_appendm(&values, propval);
1451+
propval = NULL;
14401452
break;
14411453

14421454
case VCARD_FN_PROPERTY:
14431455
cdata->fullname = propval;
1456+
strarray_appendm(&values, propval);
1457+
propval = NULL;
14441458
break;
14451459

14461460
case VCARD_NICKNAME_PROPERTY:
14471461
if (buf_len(&nicknames)) buf_putc(&nicknames, ',');
14481462
buf_appendcstr(&nicknames, propval);
1449-
cdata->nickname = buf_cstring(&nicknames);;
1463+
cdata->nickname = buf_cstring(&nicknames);
14501464
break;
14511465

14521466
case VCARD_EMAIL_PROPERTY: {
@@ -1471,8 +1485,9 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb,
14711485
}
14721486
}
14731487
}
1474-
strarray_append(&emails, propval);
1488+
strarray_appendm(&emails, propval);
14751489
strarray_append(&emails, ispref ? "1" : "");
1490+
propval = NULL;
14761491
break;
14771492
}
14781493

@@ -1511,6 +1526,8 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb,
15111526
default:
15121527
break;
15131528
}
1529+
1530+
free(propval);
15141531
}
15151532

15161533
int r;
@@ -1529,6 +1546,7 @@ EXPORTED int carddav_writecard_x(struct carddav_db *carddavdb,
15291546
if (!r) r = carddav_write_groups(carddavdb, cdata->dav.rowid, &member_uids);
15301547

15311548
buf_free(&nicknames);
1549+
strarray_fini(&values);
15321550
strarray_fini(&emails);
15331551
strarray_fini(&member_uids);
15341552

Diff for: ‎imap/http_carddav.c

+21-11
Original file line numberDiff line numberDiff line change
@@ -602,9 +602,10 @@ static int carddav_store_resource(struct transaction_t *txn,
602602
{
603603
vcardproperty *prop;
604604
struct carddav_data *cdata;
605-
const char *version = NULL, *uid = NULL, *fullname = NULL;
605+
char *version = NULL, *uid = NULL, *fullname = NULL;
606606
struct index_record *oldrecord = NULL, record;
607607
char *mimehdr;
608+
int r;
608609

609610
/* Validate the vCard data */
610611
if (!vcard) {
@@ -620,15 +621,15 @@ static int carddav_store_resource(struct transaction_t *txn,
620621

621622
switch (vcardproperty_isa(prop)) {
622623
case VCARD_VERSION_PROPERTY:
623-
version = propval;
624+
version = xstrdup(propval);
624625
break;
625626

626627
case VCARD_UID_PROPERTY:
627-
uid = propval;
628+
uid = xstrdup(propval);
628629
break;
629630

630631
case VCARD_FN_PROPERTY:
631-
fullname = propval;
632+
fullname = xstrdup(propval);
632633
break;
633634

634635
default:
@@ -666,7 +667,8 @@ static int carddav_store_resource(struct transaction_t *txn,
666667
if (buf_len(buf) > max_size) {
667668
buf_destroy(buf);
668669
txn->error.precond = CARDDAV_MAX_SIZE;
669-
return HTTP_FORBIDDEN;
670+
r = HTTP_FORBIDDEN;
671+
goto done;
670672
}
671673

672674
/* Create and cache RFC 5322 header fields for resource */
@@ -694,10 +696,16 @@ static int carddav_store_resource(struct transaction_t *txn,
694696
spool_remove_header(xstrdup("Content-Description"), txn->req_hdrs);
695697

696698
/* Store the resource */
697-
int r = dav_store_resource(txn, buf_cstring(buf), 0,
698-
mailbox, oldrecord, cdata->dav.createdmodseq,
699-
NULL, NULL);
699+
r = dav_store_resource(txn, buf_cstring(buf), 0,
700+
mailbox, oldrecord, cdata->dav.createdmodseq,
701+
NULL, NULL);
700702
buf_destroy(buf);
703+
704+
done:
705+
free(version);
706+
free(uid);
707+
free(fullname);
708+
701709
return r;
702710
}
703711

@@ -1363,6 +1371,7 @@ static int carddav_put(struct transaction_t *txn, void *obj,
13631371
{
13641372
struct carddav_db *db = (struct carddav_db *)destdb;
13651373
vcardcomponent *vcard = (vcardcomponent *)obj;
1374+
char *uid = NULL, *fullname = NULL;
13661375
char *type = NULL, *subtype = NULL;
13671376
struct param *params = NULL;
13681377
const char *want_ver = NULL;
@@ -1446,7 +1455,6 @@ static int carddav_put(struct transaction_t *txn, void *obj,
14461455

14471456
/* Sanity check vCard data */
14481457
vcardproperty *prop;
1449-
const char *uid = NULL, *fullname = NULL;
14501458
for (prop = vcardcomponent_get_first_property(vcard, VCARD_ANY_PROPERTY);
14511459
prop;
14521460
prop = vcardcomponent_get_next_property(vcard, VCARD_ANY_PROPERTY)) {
@@ -1469,11 +1477,11 @@ static int carddav_put(struct transaction_t *txn, void *obj,
14691477
break;
14701478

14711479
case VCARD_UID_PROPERTY:
1472-
uid = propval;
1480+
uid = xstrdup(propval);
14731481
break;
14741482

14751483
case VCARD_FN_PROPERTY:
1476-
fullname = propval;
1484+
fullname = xstrdup(propval);
14771485
break;
14781486

14791487
default:
@@ -1527,6 +1535,8 @@ static int carddav_put(struct transaction_t *txn, void *obj,
15271535

15281536
done:
15291537
param_free(&params);
1538+
free(uid);
1539+
free(fullname);
15301540
free(subtype);
15311541
free(type);
15321542

0 commit comments

Comments
 (0)