Skip to content
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.

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

Fix: Memory exhaustion issue in dol_htmlentitiesbr() #33066

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

GregoireIvars
Copy link

FIx: Memory exhaustion when comment in a ticket is too long #33065

Description :

When writing a message or comment in a ticket, if it turns out to be too long, a memory error occurs. I have implemented a fix for this case

This PR fixes a memory exhaustion issue in the dol_htmlentitiesbr() function.

  • Increased memory limit to prevent crashes.
  • Added input size verification to avoid excessive memory usage.
  • Logged input size for debugging large strings.
    @defrance

@@ -8723,6 +8723,9 @@ function dol_htmlentitiesbr($stringtoencode, $nl2brmode = 0, $pagecodefrom = 'UT
$newstring = strtr($newstring, array('&' => '__PROTECTand__', '<' => '__PROTECTlt__', '>' => '__PROTECTgt__', '"' => '__PROTECTdquot__'));
$newstring = dol_htmlentities($newstring, ENT_COMPAT, $pagecodefrom); // Make entity encoding
$newstring = strtr($newstring, array('__PROTECTand__' => '&', '__PROTECTlt__' => '<', '__PROTECTgt__' => '>', '__PROTECTdquot__' => '"'));
if (strlen($stringtoencode) > 1000000) {// limit, to avoid memory problem
return htmlentities(substr($stringtoencode, 0, 1000000)) . '...';
Copy link
Member

@eldy eldy Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this will lead to security vulnerability because you truncate $stringtoencode instead of truncating the $newstring that has been sanitized.
Why not just doing

if (strlen($newstring) > 1000000) {// limit, to avoid memory problem
			return substr($newstring, 0, 1000000) . '...';
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback, I made the changes you suggested!

@eldy eldy added the Discussion Some questions or discussions are opened and wait answers of author or other people to be processed label Feb 13, 2025
@hregis
Copy link
Contributor

hregis commented Feb 13, 2025

@GregoireIvars @defrance it's not a limitation of your server ? Maybe add a constant for this ?

@hregis
Copy link
Contributor

hregis commented Feb 14, 2025

@eldy @GregoireIvars @defrance je ne comprend pas cette limitation ?! est-ce que qu'au fond ça ne dépend pas des limitation du serveur ? est-ce que ceci ne va pas limiter ceux qui ont la puissance de gérer ceci ? (je dis ça je dis rien !) ;-)

@eldy
Copy link
Member

eldy commented Feb 14, 2025

@eldy @GregoireIvars @defrance je ne comprend pas cette limitation ?! est-ce que qu'au fond ça ne dépend pas des limitation du serveur ? est-ce que ceci ne va pas limiter ceux qui ont la puissance de gérer ceci ? (je dis ça je dis rien !) ;-)

Yes this looks strange. When there is a memory limit reached, there is a php fatal error, not an infinite delay with no answer.
@GregoireIvars do you know what was size of the string before truncation ? And what is your php max size of memory ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Some questions or discussions are opened and wait answers of author or other people to be processed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants