Request 2798 (accepted)

Overview

Request 2798 (accepted)

Fix memory-safety bugs

Created by sicherha over 3 years ago
In state accepted
Package maintainer: mollekopf
Open review for Admin
Open review for kolab-developers

Submit libkolab

Submit package home:sicherha:branches:Kolab:16 / libkolab to package Kolab:Winterfell / libkolab

libkolab.spec Changed

0001-Fix-dangling-pointers.patch Added

@@ -0,0 +1,64 @@
+From 4bda8a1cb3befa5dc4b03c6590640053afb74ef5 Mon Sep 17 00:00:00 2001
+From: Christoph Erhardt <kolab@sicherha.de>
+Date: Tue, 25 May 2021 08:57:22 +0200
+Subject: [PATCH 1/2] Fix dangling pointers
+
+std::vector gives zero guarantees that pointers to its elements remain
+valid when the vector's size changes. In particular, pushing new
+elements into the vector may trigger reallocation of the underlying heap
+area.
+
+Consequently, Event::delegate() needs to ensure that any modifications
+to the d->attendees vector are performed before pointers to its elements
+are taken and collected.
+
+Found with Valgrind.
+
+
+Reviewers: mollekopf
+
+Reviewed By: mollekopf
+
+Subscribers: mollekopf
+
+Differential Revision: https://git.kolab.org/D2548
+---
+ calendaring/event.cpp | 17 +++++++++++------
+ 1 file changed, 11 insertions(+), 6 deletions(-)
+
+diff --git a/calendaring/event.cpp b/calendaring/event.cpp
+index bcdf833..ea9ead1 100644
+--- a/calendaring/event.cpp
++++ b/calendaring/event.cpp
+@@ -76,17 +76,22 @@ bool contains(const Kolab::ContactReference &delegatorRef, const std::vector <Ko
+ void Event::delegate(const std::vector< Attendee >& delegators, const std::vector< Attendee >& delegatees)
+ {
+ 
+-    //First build a list of attendee references, and insert any missing attendees
+-    std::vector<Kolab::Attendee*> delegateesRef;
++    //First insert any missing attendees
+     foreach(const Attendee &a, delegatees) {
+-        if (Attendee *attendee = getAttendee(a.contact())) {
+-            delegateesRef.push_back(attendee);
+-        } else {
++        if (!getAttendee(a.contact())) {
+             d->attendees.push_back(a);
+-            delegateesRef.push_back(&d->attendees.back());
+         }
+     }
+ 
++    //Build a list of attendee references
++    //These are pointers into d->attendees, so we MUST NOT modify that vector after this point!
++    std::vector<Kolab::Attendee*> delegateesRef;
++    foreach(const Attendee &a, delegatees) {
++        Attendee *attendee = getAttendee(a.contact());
++        Q_ASSERT(attendee);
++        delegateesRef.push_back(attendee);
++    }
++
+     std::vector<Kolab::Attendee*> delegatorsRef;
+     foreach(const Attendee& a, delegators) {
+         if (Attendee *attendee = getAttendee(a.contact())) {
+-- 
+2.31.1
+

0002-Fixed-memory-leaks-in-tests.patch Added

@@ -0,0 +1,196 @@
+From 483b1019396296dfffa634abc80ddd19b61b9aa9 Mon Sep 17 00:00:00 2001
+From: Christoph Erhardt <kolab@sicherha.de>
+Date: Tue, 25 May 2021 09:01:44 +0200
+Subject: [PATCH 2/2] Fixed memory leaks in tests
+
+toString requires the returned memory to be freed.
+
+Reviewers: mollekopf
+
+Reviewed By: mollekopf
+
+Subscribers: mollekopf
+
+Differential Revision: https://git.kolab.org/D2548
+---
+ tests/calendaringtest.cpp |  4 +-
+ tests/testhelpers.h       | 77 +++++++++++++++++++++++----------------
+ 2 files changed, 48 insertions(+), 33 deletions(-)
+
+diff --git a/tests/calendaringtest.cpp b/tests/calendaringtest.cpp
+index 44440dc..cc65605 100644
+--- a/tests/calendaringtest.cpp
++++ b/tests/calendaringtest.cpp
+@@ -110,7 +110,9 @@ void CalendaringTest::testRecurrence()
+     Kolab::cDateTime previousDate = event.start();
+     for (int i = 0; i < 9; i++) {
+         const Kolab::cDateTime nextDate = event.getNextOccurence(previousDate);
+-        qDebug() << QTest::toString(nextDate);
++        const char *nextDateString = QTest::toString(nextDate);
++        qDebug() << nextDateString;
++        delete[] nextDateString;
+         QCOMPARE(nextDate, Kolab::cDateTime("Europe/Zurich", previousDate.year(), previousDate.month(), previousDate.day()+1, previousDate.hour(), previousDate.minute(), previousDate.second()));
+         const Kolab::cDateTime endDate = event.getOccurenceEndDate(nextDate);
+ //         qDebug() << QTest::toString(endDate);
+diff --git a/tests/testhelpers.h b/tests/testhelpers.h
+index e09dd3a..8a443de 100644
+--- a/tests/testhelpers.h
++++ b/tests/testhelpers.h
+@@ -44,6 +44,19 @@ Q_DECLARE_METATYPE(KCalCore::Event);
+ Q_DECLARE_METATYPE(KCalCore::Todo);
+ Q_DECLARE_METATYPE(KCalCore::Journal);
+ 
++namespace {
++
++    // Appends a C string to a QByteArray and delete[]s the string argument.
++    // Returns a reference to the QByteArray.
++    QByteArray &append(QByteArray &ba, const char *string)
++    {
++        ba += QByteArray(string);
++        delete[] string;
++        return ba;
++    }
++
++}
++
+ namespace QTest {
+     
+     template<>
+@@ -123,7 +136,7 @@ namespace QTest {
+     {
+         QByteArray ba = "KCalCore::DateTimeList(";
+         foreach(const QDateTime &i, l) {
+-            ba += toString(i);
++            append(ba, toString(i));
+         }
+         ba += ")";
+         return qstrdup(ba.data());
+@@ -145,20 +158,20 @@ namespace QTest {
+             ba += QByteArray::number(r->recurrenceType()) + "\n";
+             ba += QByteArray::number(r->frequency()) + "\n";
+             ba += QByteArray::number(r->duration()) + "\n";
+-            ba += QByteArray(toString(r->startDt())) + "\n";
+-            ba += QByteArray(toString(r->endDt())) + "\n";
+-            ba += QByteArray(toString(r->bySeconds())) + "\n";
+-            ba += QByteArray(toString(r->byMinutes())) + "\n";
+-            ba += QByteArray(toString(r->byHours())) + "\n";
+-            ba += QByteArray(toString(r->byDays())) + "\n";
+-            ba += QByteArray(toString(r->byMonthDays())) + "\n";
+-            ba += QByteArray(toString(r->byYearDays())) + "\n";
+-            ba += QByteArray(toString(r->byMonths())) + "\n";
++            append(ba, toString(r->startDt())) += "\n";
++            append(ba, toString(r->endDt())) += "\n";
++            append(ba, toString(r->bySeconds())) += "\n";
++            append(ba, toString(r->byMinutes())) += "\n";
++            append(ba, toString(r->byHours())) += "\n";
++            append(ba, toString(r->byDays())) += "\n";
++            append(ba, toString(r->byMonthDays())) += "\n";
++            append(ba, toString(r->byYearDays())) += "\n";
++            append(ba, toString(r->byMonths())) += "\n";
+             ba += ")\n";
+-            ba += QByteArray(toString(at.exDates())) + "\n";
+-            ba += QByteArray(toString(at.exDateTimes())) + "\n";
+-            ba += QByteArray(toString(at.rDates())) + "\n";
+-            ba += QByteArray(toString(at.rDateTimes())) + "\n";
++            append(ba, toString(at.exDates())) += "\n";
++            append(ba, toString(at.exDateTimes())) += "\n";
++            append(ba, toString(at.rDates())) += "\n";
++            append(ba, toString(at.rDateTimes())) += "\n";
+ 
+         }
+         return qstrdup(ba.data());
+@@ -173,15 +186,15 @@ namespace QTest {
+         ba += QByteArray::number(at.frequency()) + "\n";
+         ba += QByteArray::number(at.interval()) + "\n";
+         ba += QByteArray::number(at.count()) + "\n";
+-        ba += QByteArray(toString(at.end())) + "\n";
+-        ba += QByteArray(toString(at.bysecond())) + "\n";
+-        ba += QByteArray(toString(at.byminute())) + "\n";
+-        ba += QByteArray(toString(at.byhour())) + "\n";
+-        ba += QByteArray(toString(at.byday())) + "\n";
+-        ba += QByteArray(toString(at.bymonthday())) + "\n";
+-        ba += QByteArray(toString(at.byyearday())) + "\n";
+-        ba += QByteArray(toString(at.byweekno())) + "\n";
+-        ba += QByteArray(toString(at.bymonth())) + "\n";
++        append(ba, toString(at.end())) += "\n";
++        append(ba, toString(at.bysecond())) += "\n";
++        append(ba, toString(at.byminute())) += "\n";
++        append(ba, toString(at.byhour())) += "\n";
++        append(ba, toString(at.byday())) += "\n";
++        append(ba, toString(at.bymonthday())) += "\n";
++        append(ba, toString(at.byyearday())) += "\n";
++        append(ba, toString(at.byweekno())) += "\n";
++        append(ba, toString(at.bymonth())) += "\n";
+         ba += ")";
+         return qstrdup(ba.data());
+     }
+@@ -213,7 +226,7 @@ namespace QTest {
+     {
+         QByteArray ba = "vector<Kolab::ContactReference>(";
+         for (std::size_t i = 0; i < v.size(); i++) {
+-                ba += QByteArray(toString(v.at(i)))+ "\n";
++                append(ba, toString(v.at(i))) += "\n";
+             }
+             ba += ")";
+             return qstrdup(ba.data());
+@@ -229,8 +242,8 @@ namespace QTest {
+         ba += QByteArray::number(a.role())  + "\n";
+         ba += QByteArray::number(a.rsvp())  + "\n";
+         ba += QByteArray::fromStdString(a.contact().uid())+"\n";
+-        ba += QByteArray(toString(a.delegatedTo()))+"\n";
+-        ba += QByteArray(toString(a.delegatedFrom()))+ "\n";
++        append(ba, toString(a.delegatedTo())) += "\n";
++        append(ba, toString(a.delegatedFrom())) += "\n";
+         ba += QByteArray::number(a.cutype())+ "\n";
+         ba += ")";
+         return qstrdup(ba.data());
+@@ -241,7 +254,7 @@ namespace QTest {
+     {
+         QByteArray ba = "vector<Kolab::Attendee>(";
+         for (std::size_t i = 0; i < v.size(); i++) {
+-            ba += QByteArray(toString(v.at(i)))+ "\n";
++            append(ba, toString(v.at(i))) += "\n";
+             ba += QByteArray("#######################")+ "\n";
+         }
+         ba += ")";
+@@ -263,7 +276,7 @@ namespace QTest {
+     {
+         QByteArray ba = "vector<Kolab::CustomProperty>(";
+         for (std::size_t i = 0; i < v.size(); i++) {
+-            ba += QByteArray(toString(v.at(i)))+ "\n";
++            append(ba, toString(v.at(i))) += "\n";
+         }
+         ba += ")";
+         return qstrdup(ba.data());
+@@ -273,8 +286,8 @@ namespace QTest {
+     char *toString(const Kolab::Period &p)
+     {
+         QByteArray ba = "Kolab::Period(";
+-        ba += QByteArray(toString(p.start))+ "\n";
+-        ba += QByteArray(toString(p.end))+ "\n";
++        append(ba, toString(p.start)) += "\n";
++        append(ba, toString(p.end)) += "\n";
+         ba += ")";
+         return qstrdup(ba.data());
+     }
+@@ -284,7 +297,7 @@ namespace QTest {
+     {
+         QByteArray ba = "vector<Kolab::Period>(";
+         for (std::size_t i = 0; i < v.size(); i++) {
+-            ba += QByteArray(toString(v.at(i)))+ "\n";
++            append(ba, toString(v.at(i))) += "\n";
+         }
+         ba += ")";
+         return qstrdup(ba.data());
+@@ -298,7 +311,7 @@ namespace QTest {
+         ba += QByteArray::fromStdString(p.eventUid())+ "\n";
+         ba += QByteArray::fromStdString(p.eventLocation())+ "\n";
+         ba += QByteArray::fromStdString(p.eventSummary())+ "\n";
+-        ba += QByteArray(toString(p.periods()))+ "\n";
++        append(ba, toString(p.periods())) += "\n";
+         ba += ")";
+         return qstrdup(ba.data());
+     }
+-- 
+2.31.1
+

debian.changelog Changed

debian.series Changed

libkolab.dsc Changed

Build Results
RPM Lint

Refresh

Comments for request 2798 0

Request History

sicherha created request over 3 years ago

Fix memory-safety bugs

sicherha accepted request over 3 years ago