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 LogRecord "extra" data leaking between handlers #1819

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Fix LogRecord "extra" data leaking between handlers #1819

merged 2 commits into from
Oct 27, 2023

Conversation

cosminardeleanu
Copy link
Contributor

@cosminardeleanu cosminardeleanu commented Jul 7, 2023

Context:

  • Let's say i have 3 handlers attached to monolog logger: file handler, slack handler, elastic search handler.
    • Note that each handler can push its own processor, which can alter the LogRecord entity.
  • On the elastic search handler, we need extra info, so push a processor to add them, (stupid example: $logRecord->extra['new_entry_just_for_elastic_search'] = 'stuff';
    • This "extra" data i also available for file handler and slack handler.
    • Basically i will get in slack handler and file handler, this extra that was supposed to be only for elastic search handler.

Why this is happening?

  • LogRecord is passed by reference, to each handler.
  • LogRecord accepts to alter data from extra, so if one handlers manages to alter this object, rest of remaining handlers, will use the altered object.

@cosminardeleanu
Copy link
Contributor Author

@Seldaek ^^

@aistis-
Copy link

aistis- commented Oct 3, 2023

I confirm the issue persists with 3.4.0. Waiting for the merge 🤞

@stof
Copy link
Contributor

stof commented Oct 24, 2023

@Seldaek with the migration to an object in Monolog 3, this is indeed an issue for handler-level processors rather than logger-level processors. Should the logger clone the record before calling the handler ?

@Seldaek
Copy link
Owner

Seldaek commented Oct 25, 2023

Yeah I think it'd be better to clone.. both in Logger and in GroupHandler and related handlers which fan out records to nested handlers. Otherwise the problem is just shifted one problem lower.

@Seldaek Seldaek added this to the 3.x milestone Oct 25, 2023
@Seldaek Seldaek added the Bug label Oct 25, 2023
@stof
Copy link
Contributor

stof commented Oct 25, 2023

indeed, fan-out handlers also need to clone.

@Seldaek
Copy link
Owner

Seldaek commented Oct 27, 2023

Alright I think with my commit it should take care of it for all cases. I hope I didn't forget any fan-out handler.

@Seldaek Seldaek merged commit b0f4bf7 into Seldaek:main Oct 27, 2023
11 checks passed
@cosminardeleanu cosminardeleanu deleted the fix-log-record-data-leaking-because-of-object-reference branch October 30, 2023 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants