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

Handle the case when pid replacement is not needed #73664

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

cshung
Copy link
Member

@cshung cshung commented Aug 9, 2022

Fixing a case I missed in #70764. In case {pid} is not found in the file name, the existing code would use an uninitialized buffer, which is not good.

@cshung cshung added this to the 7.0.0 milestone Aug 9, 2022
@cshung cshung self-assigned this Aug 9, 2022
@ghost
Copy link

ghost commented Aug 9, 2022

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixing a case I missed in #70764. In case {pid} is not found in the file name, the existing code would use an uninitialized buffer, which is not good.

Author: cshung
Assignees: cshung
Labels:

area-GC-coreclr

Milestone: 7.0.0

Copy link
Member

@mrsharm mrsharm left a comment

Choose a reason for hiding this comment

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

LGTM!

@cshung cshung removed the request for review from noahfalk August 10, 2022 23:09
@cshung cshung merged commit 7c9b252 into dotnet:main Aug 10, 2022
@cshung cshung deleted the public/fix-replace-pid branch August 10, 2022 23:09
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

The new function looks potentially error prone for future usage, I'd suggest changing it to remove the sharp edges.

}
else
{
return logFilename;
Copy link
Member

Choose a reason for hiding this comment

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

The memory management rules of doing this feel error prone if sometimes we are returning a pointer to one buffer and other times we are returning a pointer to a different buffer. I'd suggest copying logFilename to fileName, make the return type void again, and the caller will always use fileName as before.

I don't think any of the callers that currently exist have an issue here, but I worry that it would be easy to create a new caller in the future that did have an issue.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants