Skip to content

Commit

Permalink
icalcomponent.c: do not dereference NULL when comparing components
Browse files Browse the repository at this point in the history 
The icalcomponent_normalize function sorts components by comparing
their mandatory property values. This causes it to dereference a
NULL pointer if one of the compared components erroneously is
missing such a property.

This patch fixes that by checking for the presence of mandatory
properties before comparison, otherwise sorting components having
a property before ones that don't.
  • Loading branch information
@rsto
rsto authored and Allen Winter committed May 28, 2024
1 parent 1962517 commit 8a04e0b
Show file tree
Hide file tree
Showing 2 changed files with 188 additions and 16 deletions.
68 changes: 52 additions & 16 deletions src/libical/icalcomponent.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2484,6 +2484,15 @@ static int prop_compare(void *a, void *b)
return r;
}

static inline int compare_nullptr(const void *a, const void *b)
{
if (!a == !b)
return 0;

// non-NULL sorts before NULL
return a ? -1 : 1;
}

static int comp_compare(void *a, void *b)
{
icalcomponent *c1 = (icalcomponent *)a;
Expand Down Expand Up @@ -2517,26 +2526,38 @@ static int comp_compare(void *a, void *b)
ICAL_TRIGGER_PROPERTY);
p2 = icalcomponent_get_first_property(c2,
ICAL_TRIGGER_PROPERTY);
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));

if (r == 0) {
p1 = icalcomponent_get_first_property(c1,
ICAL_ACTION_PROPERTY);
p2 = icalcomponent_get_first_property(c2,
ICAL_ACTION_PROPERTY);
if (p1 && p2) {
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));
if (r == 0) {
p1 = icalcomponent_get_first_property(c1,
ICAL_ACTION_PROPERTY);
p2 = icalcomponent_get_first_property(c2,
ICAL_ACTION_PROPERTY);
if (p1 && p2) {
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));
} else {
r = compare_nullptr(p1, p2);
}
}
} else {
r = compare_nullptr(p1, p2);
}

break;

case ICAL_VTIMEZONE_COMPONENT:
p1 = icalcomponent_get_first_property(c1,
ICAL_TZID_PROPERTY);
p2 = icalcomponent_get_first_property(c2,
ICAL_TZID_PROPERTY);
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));
if (p1 && p2) {
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));
} else {
r = compare_nullptr(p1, p2);
}
break;

case ICAL_XSTANDARD_COMPONENT:
Expand All @@ -2545,26 +2566,41 @@ static int comp_compare(void *a, void *b)
ICAL_DTSTART_PROPERTY);
p2 = icalcomponent_get_first_property(c2,
ICAL_DTSTART_PROPERTY);
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));

if (p1 && p2) {
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));
} else {
r = compare_nullptr(p1, p2);
}
break;

case ICAL_VVOTER_COMPONENT:
p1 = icalcomponent_get_first_property(c1,
ICAL_VOTER_PROPERTY);
p2 = icalcomponent_get_first_property(c2,
ICAL_VOTER_PROPERTY);
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));

if (p1 && p2) {
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));
} else {
r = compare_nullptr(p1, p2);
}
break;

case ICAL_XVOTE_COMPONENT:
p1 = icalcomponent_get_first_property(c1,
ICAL_POLLITEMID_PROPERTY);
p2 = icalcomponent_get_first_property(c2,
ICAL_POLLITEMID_PROPERTY);
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));

if (p1 && p2) {
r = strcmp(icalproperty_get_value_as_string(p1),
icalproperty_get_value_as_string(p2));
} else {
r = compare_nullptr(p1, p2);
}
break;

default:
Expand Down
136 changes: 136 additions & 0 deletions src/test/regression.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4913,6 +4913,141 @@ void test_icalcomponent_normalize(void)
str_is("Normalized components match", calStr1, calStr2);
}

void test_icalcomponent_normalize_missing_mandatory_props(void)
{
// This test asserts that normalize does not crash when comparing
// components which do not have mandatory properties set. If one
// component has the property set and the other does not, then
// the component having the property sorts first.

struct testcase {
const char *calStr;
const char *expect;
} tests[] = {{.calStr =
"BEGIN:VEVENT\r\n"
"BEGIN:VALARM\r\n"
// no ACTION
// no TRIGGER
"END:VALARM\r\n"
"BEGIN:VALARM\r\n"
"ACTION:DISPLAY\r\n"
"TRIGGER:PT0S\r\n"
"END:VALARM\r\n"
"END:VEVENT\r\n",
.expect =
"BEGIN:VEVENT\r\n"
"BEGIN:VALARM\r\n"
"ACTION:DISPLAY\r\n"
"TRIGGER:PT0S\r\n"
"END:VALARM\r\n"
"BEGIN:VALARM\r\n"
// no ACTION
// no TRIGGER
"END:VALARM\r\n"
"END:VEVENT\r\n"},
{.calStr =
"BEGIN:VEVENT\r\n"
"BEGIN:VALARM\r\n"
// no ACTION
"TRIGGER:PT0S\r\n"
"END:VALARM\r\n"
"BEGIN:VALARM\r\n"
"ACTION:DISPLAY\r\n"
"TRIGGER:PT0S\r\n"
"END:VALARM\r\n"
"END:VEVENT\r\n",
.expect =
"BEGIN:VEVENT\r\n"
"BEGIN:VALARM\r\n"
"ACTION:DISPLAY\r\n"
"TRIGGER:PT0S\r\n"
"END:VALARM\r\n"
"BEGIN:VALARM\r\n"
// no ACTION
"TRIGGER:PT0S\r\n"
"END:VALARM\r\n"
"END:VEVENT\r\n"},
{
.calStr =
"BEGIN:VCALENDAR\r\n"
"BEGIN:VTIMEZONE\r\n"
// no TZID
"BEGIN:STANDARD\r\n"
"DTSTART:16010101T000000\r\n"
"TZOFFSETFROM:-0200\r\n"
"TZOFFSETTO:-0200\r\n"
"END:STANDARD\r\n"
"END:VTIMEZONE\r\n"
"BEGIN:VTIMEZONE\r\n"
"TZID:/test\r\n"
"BEGIN:STANDARD\r\n"
"DTSTART:16010101T000000\r\n"
"TZOFFSETFROM:-0200\r\n"
"TZOFFSETTO:-0200\r\n"
"END:STANDARD\r\n"
"END:VTIMEZONE\r\n"
"END:VCALENDAR\r\n",
.expect =
"BEGIN:VCALENDAR\r\n"
"BEGIN:VTIMEZONE\r\n"
"TZID:/test\r\n"
"BEGIN:STANDARD\r\n"
"DTSTART:16010101T000000\r\n"
"TZOFFSETFROM:-0200\r\n"
"TZOFFSETTO:-0200\r\n"
"END:STANDARD\r\n"
"END:VTIMEZONE\r\n"
"BEGIN:VTIMEZONE\r\n"
// no TZID
"BEGIN:STANDARD\r\n"
"DTSTART:16010101T000000\r\n"
"TZOFFSETFROM:-0200\r\n"
"TZOFFSETTO:-0200\r\n"
"END:STANDARD\r\n"
"END:VTIMEZONE\r\n"
"END:VCALENDAR\r\n",
},
{.calStr =
"BEGIN:VTIMEZONE\r\n"
"BEGIN:STANDARD\r\n"
// no DTSTART
"TZOFFSETFROM:-0200\r\n"
"TZOFFSETTO:-0200\r\n"
"END:STANDARD\r\n"
"BEGIN:STANDARD\r\n"
"DTSTART:16010101T000000\r\n"
"TZOFFSETFROM:-0200\r\n"
"TZOFFSETTO:-0200\r\n"
"END:STANDARD\r\n"
"END:VTIMEZONE\r\n",
.expect =
"BEGIN:VTIMEZONE\r\n"
"BEGIN:STANDARD\r\n"
"DTSTART:16010101T000000\r\n"
"TZOFFSETFROM:-0200\r\n"
"TZOFFSETTO:-0200\r\n"
"END:STANDARD\r\n"
"BEGIN:STANDARD\r\n"
// no DTSTART
"TZOFFSETFROM:-0200\r\n"
"TZOFFSETTO:-0200\r\n"
"END:STANDARD\r\n"
"END:VTIMEZONE\r\n"},
{NULL, NULL}};

for (const struct testcase *tc = tests; tc->calStr; tc++) {
icalcomponent *ical = icalcomponent_new_from_string(tc->calStr);
icalcomponent_normalize(ical);
const char *s = icalcomponent_as_ical_string(ical);
icalcomponent_free(ical);

if (VERBOSE) {
printf("iCal:\n%s\n\n", s);
}
str_is("Normalized component matches", s, tc->expect);
}
}

static void test_builtin_compat_tzid(void)
{
struct _cases {
Expand Down Expand Up @@ -5592,6 +5727,7 @@ int main(int argc, char *argv[])
test_run("Test icalarray_sort", test_icalarray_sort, do_test, do_header);

test_run("Test icalcomponent_normalize", test_icalcomponent_normalize, do_test, do_header);
test_run("Test icalcomponent_normalize_missing_mandatory_props", test_icalcomponent_normalize_missing_mandatory_props, do_test, do_header);
test_run("Test builtin compat TZID", test_builtin_compat_tzid, do_test, do_header);
test_run("Test VCC vCard parse", test_vcc_vcard_parse, do_test, do_header);
test_run("Test implicit DTEND and DURATION for VEVENT and VTODO", test_implicit_dtend_duration, do_test, do_header);
Expand Down

0 comments on commit 8a04e0b

Please sign in to comment.