Skip to content

[Mime] Fix memory leak #51874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 10, 2023
Merged

[Mime] Fix memory leak #51874

merged 1 commit into from
Oct 10, 2023

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Oct 6, 2023 •
edited
Loading

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #51764
License MIT

fabpot
fabpot commented Oct 6, 2023
View reviewed changes
@fabpot fabpot force-pushed the mime-message-memory-fix branch from 3fa982c to ed0d833 Compare October 6, 2023 16:53
@fabpot fabpot changed the base branch from 5.4 to 6.4 October 6, 2023 16:54
@fabpot fabpot force-pushed the mime-message-memory-fix branch from ed0d833 to 606ee03 Compare October 6, 2023 16:57
@fabpot
Copy link
Member Author

fabpot commented Oct 6, 2023

Fixing it is not possible in 5.4.
I propose to deprecate the possibility of sending a message with a closed generator in 6.4 and throw in 7.0. That way, we don't need to store the generator result and that avoids the memory leak.

@stof
Copy link
Member

stof commented Oct 6, 2023

@fabpot the issue is not just about sending a message with a closed generator. If you try to both access the string content and serialize the object, this will make 2 usages of the iterator, with the RawMessage being the one closing the generator in the first usage.

@OskarStark OskarStark mentioned this pull request Oct 6, 2023
Closed
@fabpot
Copy link
Member Author

fabpot commented Oct 6, 2023

@stof Sure, but that's not something people will do. What would you suggest?

@chalasr
Copy link
Member

chalasr commented Oct 7, 2023

Isn't patching only toIterable() enough to fix the issue?

diff --git a/src/Symfony/Component/Mime/RawMessage.php b/src/Symfony/Component/Mime/RawMessage.php
index d2a311daeb..14223d3d7f 100644
--- a/src/Symfony/Component/Mime/RawMessage.php
+++ b/src/Symfony/Component/Mime/RawMessage.php
@@ -48,12 +48,9 @@ class RawMessage implements \Serializable
             return;
         }
 
-        $message = '';
         foreach ($this->message as $chunk) {
-            $message .= $chunk;
             yield $chunk;
         }
-        $this->message = $message;
     }
 
     /**

Transforming the generator to an array in toString() seems unavoidable, unless we forbid calling toString() more than once which sounds worse.

@fabpot fabpot force-pushed the mime-message-memory-fix branch from 606ee03 to ee81ae4 Compare October 10, 2023 06:07
@fabpot fabpot merged commit d6c8797 into symfony:6.4 Oct 10, 2023
@fabpot fabpot deleted the mime-message-memory-fix branch October 10, 2023 06:10
@fabpot fabpot mentioned this pull request Oct 10, 2023
Merged
fabpot added a commit that referenced this pull request Oct 15, 2023
@fabpot
feature #51926 [Mime] Forbid messages that are generators to be used …
c6a9dde 
…more than once (fabpot)

This PR was merged into the 6.4 branch.

Discussion
----------

[Mime] Forbid messages that are generators to be used more than once

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #51764, Fix #51874
| License       | MIT

This is a continuation of #51874, when someone tries to send the same message more than once.
`@chalasr` `@stof`

Commits
-------

3c5fe51 [Mime] Forbid messages that are generators to be used more than once
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyrixx lyrixx Awaiting requested review from lyrixx

@yceruto yceruto Awaiting requested review from yceruto

@chalasr chalasr Awaiting requested review from chalasr

@dunglas dunglas Awaiting requested review from dunglas

@OskarStark OskarStark Awaiting requested review from OskarStark

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory Leak in Symfony Mailer when sending many attachments of >5MB
3 participants
@fabpot @stof @chalasr