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

Add self-assignment cases in SocketAddress #56718

Closed
wooffie opened this issue Jan 23, 2025 · 1 comment · Fixed by #56986
Closed

Add self-assignment cases in SocketAddress #56718

wooffie opened this issue Jan 23, 2025 · 1 comment · Fixed by #56986

Comments

@wooffie
Copy link
Contributor

wooffie commented Jan 23, 2025

Version

20.18.0

Platform

any

Subsystem

src

What steps will reproduce the bug?

I think we can add check for self-assigment in ctors/assigment operators

memcpy(&address_, &addr.address_, addr.length());

Expect to see this:

SocketAddress& SocketAddress::operator=(const SocketAddress& addr) {
  if (this != &addr) {
    memcpy(&address_, &addr.address_, addr.length());
  }
  return *this;
}

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Maybe not calling memcpy

What do you see instead?

Calling memcpy

Additional information

I wanna hear your opinion about this =)

@jazelly
Copy link
Member

jazelly commented Feb 7, 2025

I think it would be an enhancement. Feel free to raise a PR. That way this will get more attention.

wooffie added a commit to wooffie/node that referenced this issue Feb 10, 2025
wooffie added a commit to wooffie/node that referenced this issue Feb 10, 2025
nodejs-github-bot pushed a commit that referenced this issue Feb 12, 2025
Fixes: #56718
PR-URL: #56986
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Feb 17, 2025
Fixes: #56718
PR-URL: #56986
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants