bluetiger9 / SmtpClient-for-Qt

An SMTP Client writen in C++ for Qt. Allows applications to send emails (MIME with text, html, attachments, inline files, etc.) via SMTP. Supports SSL and SMTP authentication.
https://github.com/bluetiger9/SmtpClient-for-Qt/wiki
GNU Lesser General Public License v2.1
449 stars 226 forks source link

Memory leak in MimeMessage #140

Closed perdrix52 closed 1 year ago

perdrix52 commented 1 year ago

MimeMessage::MimeMessage(bool createAutoMimeContent) : hEncoding(MimePart::_8Bit) { if (createAutoMimeContent) this->content = new MimeMultiPart(); }

The default ctor news a MimeMultiPart object that is never deleted.

So when I code e.g.:

MimeMessage message;

EmailAddress sender(account, "DeepSkyStackerLive");
message.setSender(sender);

EmailAddress to(addressee, "");
message.addRecipient(to);

message.setSubject(subject);

// Now add some text to the email.
// First we create a MimeText object.

MimeText text;

text.setText(subject);

// Now add it to the mail

message.addPart(&text);

a leak of 248 bytes is created

StEvUgnIn commented 1 year ago

I propose to add the following declaration in MimeMessage to call on the memory manager present in QObject derivated classes:

class SMTP_MIME_EXPORT MimeMessage : public QObject
{
   Q_OBJECT
public:
perdrix52 commented 1 year ago

That won't resolve the leak unless you change the ctor to require a QObject as parent and any items you new in MimeMessage will also need to have MimeMessage as their parent.

perdrix52 commented 1 year ago

You could default to a nullptr as parent but that would continue to leak the memory

StEvUgnIn commented 1 year ago

Have you studied the lifecycle of QObject? Members of QObject instances are deleted when you destroy the instance.

perdrix52 commented 1 year ago

A much simpler solution might be to change the definition of content in the class from: MimePart *content;

to

std::unique_ptr<MimePart> content;

perdrix52 commented 1 year ago

Yes I have BUT ONLY IF they are Children of another QOBJECT

perdrix52 commented 1 year ago

To clarify: Qt uses parent-child relationships to manage memory. If you provide the QTcpSocket object with a parent when you create it, the parent will take care of cleaning it up. The parent can be, for example, the GUI window that uses the socket. Once the window dies (i.e. is closed) the socket dies.

You can do without the parent but then indeed you have to delete the object manually.

StEvUgnIn commented 1 year ago

A much simpler solution might be to change the definition of content in the class from: MimePart *content;

to

std::unique_ptr<MimePart> content;

Smart pointers require at least C++11. Qt already had memory managed pointers before, plus QObjects have access to a static memory manager.

bluetiger9 commented 1 year ago

Fixed the memory leak in https://github.com/bluetiger9/SmtpClient-for-Qt/commit/73f1bcdd756022f9d87987762d900ffb6d29afcb, by adding a delete when createAutoMimeContent == true.