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

Do not crash if remote file does not exist #620

Merged
merged 6 commits into from
Jun 27, 2024
Merged

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Jun 21, 2024

SUMMARY

The uninitialized pointer caused pylibssh crashing when the scp.get() failed in the first invocation of libssh.ssh_scp_pull_request() (generally when the remote file did not exist, or it was not possible to read it for some reason).

This is the full log from libssh when pytest is executed directly with the -s argument:

[2024/06/21 16:04:37.620775, 2] ssh_scp_pull_request:  Received SCP request: 'scp: /tmp/pytest-of-jjelen/pytest-22/popen-gw1/test_get_missing_src0/non-existing.txt: No such file or directory'
[2024/06/21 16:04:37.620779, 1] ssh_scp_pull_request:  SCP: Warning: scp: /tmp/pytest-of-jjelen/pytest-22/popen-gw1/test_get_missing_src0/non-existing.txt: No such file or directory
free(): invalid pointer
Fatal Python error: Aborted

(running the test from tox showed just the last line with pytest crashing)

Fixes #208
Fixes #325

ISSUE TYPE
  • Bugfix Pull Request

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jun 21, 2024
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/ansible-pylibssh-620
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

tests/conftest.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

Overall, I like the fix and the test. Let's move the unrelated stuff out and merge it. I hope this will also address the flakiness of some tests...

@webknjaz
Copy link
Member

@Jakuje could you also check if this is going to help with #222 / #208?

tests/unit/scp_test.py Outdated Show resolved Hide resolved
tests/unit/scp_test.py Outdated Show resolved Hide resolved
tests/unit/scp_test.py Outdated Show resolved Hide resolved
tests/unit/scp_test.py Outdated Show resolved Hide resolved
tests/unit/scp_test.py Outdated Show resolved Hide resolved
tests/unit/scp_test.py Outdated Show resolved Hide resolved
tests/unit/scp_test.py Outdated Show resolved Hide resolved
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

@Jakuje I just realized that we need to link the change note to #325 as well, and I wanted to give you a chance to clean up the branch after my changes. So I'm approving the PR but will wait for the final touches from you before hitting merge.

@Jakuje
Copy link
Contributor Author

Jakuje commented Jun 27, 2024

@Jakuje could you also check if this is going to help with #222 / #208?

Likely the #208 but most likely not the #222 as the SFTP goes through different code path as far as I know. I will squash the fixups now.

@Jakuje
Copy link
Contributor Author

Jakuje commented Jun 27, 2024

Should be "cleaned up". Let me know if there would be some other issues.

Copy link

@KB-perByte KB-perByte left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @Jakuje

@webknjaz webknjaz enabled auto-merge June 27, 2024 14:51
@webknjaz
Copy link
Member

@Jakuje thank you! I enabled auto-merge, it'll get into devel once the CI completes.

@webknjaz webknjaz disabled auto-merge June 27, 2024 15:08
@webknjaz webknjaz merged commit eeea24f into ansible:devel Jun 27, 2024
164 checks passed
@webknjaz
Copy link
Member

@Jakuje @KB-perByte this has been released as a part of v1.2.1: https://github.com/ansible/pylibssh/releases/tag/v1.2.1 / #613 / https://pypi.org/project/ansible-pylibssh/1.2.1/.

Enjoy! 🎉

@Jakuje
Copy link
Contributor Author

Jakuje commented Jun 27, 2024

Thank you for the patience in getting this to the standard :)

@webknjaz
Copy link
Member

No problem ;) I actually enjoy helping contributors who maintain such a quality in their PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
3 participants