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

libssh: #ifdef include files accordingly #8511

Closed
wants to merge 3 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Feb 25, 2022

Reported-by: 梦终无痕
Bug: https://curl.se/mail/lib-2022-02/0131.html

@bagder bagder added build SCP/SFTP Windows labels Feb 25, 2022
@bagder bagder changed the title libssh: #ifdef two include files accordingly libssh: #ifdef include files accordingly Feb 25, 2022
@illusory-dream
Copy link
Contributor

@illusory-dream illusory-dream commented Feb 25, 2022

S_IFLNK and S_IFMT will be undefined variable without including unistd.h. Could it work as expecting if we define them manually.

@bagder
Copy link
Member Author

@bagder bagder commented Feb 25, 2022

Could it work as expecting if we define them manually.

We can define them manually, sure. But isn't there a header file you can include that has them defined? Like maybe windows.h for example?

@bagder
Copy link
Member Author

@bagder bagder commented Feb 25, 2022

This fix seems to build on Windows now, at least with mingw

lib/vssh/libssh.c Outdated Show resolved Hide resolved
@gvanem
Copy link
Contributor

@gvanem gvanem commented Feb 26, 2022

This fix seems to build on Windows now, at least with mingw

I tried this diff and it works fine on MSVC + clang-cl too; curl.exe -k sftp://sftp.**.**

drwxr-xr-x    1 root     root          188 Feb 16 07:10 .
drwxr-xr-x    1 root     root          188 Feb 16 07:10 ..
drwxr-xr-x    3 root     root           60 Feb 26 11:33 customers
drwxr-xr-x    1 root     root           76 Feb 16 07:10 dev
drwxr-xr-x    1 root     root          756 Feb 16 07:10 etc
drwxr-xr-x    1 root     root           48 Dec  4  2020 lib
...

Except the typical problem with the peer certificate.

bagder added a commit that referenced this issue Feb 26, 2022
bagder added a commit that referenced this issue Feb 26, 2022
bagder added a commit that referenced this issue Feb 26, 2022
@bagder bagder force-pushed the bagder/libssh-include branch from e2060f6 to 134d9b7 Compare Feb 26, 2022
@gvanem
Copy link
Contributor

@gvanem gvanem commented Feb 26, 2022

But I noted that WSAStartup() is now called twice:

  • In libcurl's Curl_win32_init().
  • And libssh's ssh_socket_init().

Any way to avoid that?

@bagder
Copy link
Member Author

@bagder bagder commented Feb 26, 2022

It seems tricky because libssh has no apparent way to inhibit that. ssh_socket_init() is called from within ssh_init() but there's no way for us to tell it not to.

@bagder bagder force-pushed the bagder/libssh-include branch from 134d9b7 to 59ba485 Compare Feb 26, 2022
@jay
Copy link
Member

@jay jay commented Feb 26, 2022

But I noted that WSAStartup() is now called twice:

that's fine as long as there's a corresponding cleanup call. multiple startup calls increment an internal counter and it works the same as curl_global_init/cleanup.

bagder added a commit that referenced this issue Feb 26, 2022
bagder added a commit that referenced this issue Feb 26, 2022
@bagder bagder deleted the bagder/libssh-include branch Feb 26, 2022
pghmcfc added a commit to pghmcfc/curl that referenced this issue Mar 6, 2022
The 'oldlibssh' feature indicates that the error code returned by libssh
for a broken known_hosts file should be 67 rather than 60 (test1459).
This feature was added as part of curl#8444 with 'oldlibssh' mapping to
libssh versions prior to 0.9.6, and then refined as part of curl#8511 to map
to versions prior to 0.9.5.

In Red Hat Enterprise Linux 8.5 there is a patched version of libssh
version 0.9.4 (https://git.centos.org/rpms/libssh/blob/c8/f/SOURCES)
in which test1459 fails because it returns the "new" value rather than
the "old" one. It's plausible that one of the patches is responsible
for this rather than the underlying code but I don't think so.

This change therefore drops the 'oldlibssh' version check to map to
libssh versions older than 0.9.4, which fixes builds on RHEL-8.
bagder pushed a commit that referenced this issue Mar 7, 2022
The 'oldlibssh' feature indicates that the error code returned by libssh
for a broken known_hosts file should be 67 rather than 60 (test1459).
This feature was added as part of #8444 with 'oldlibssh' mapping to
libssh versions prior to 0.9.6, and then refined as part of #8511 to map
to versions prior to 0.9.5.

In Red Hat Enterprise Linux 8.5 there is a patched version of libssh
version 0.9.4 (https://git.centos.org/rpms/libssh/blob/c8/f/SOURCES) in
which test1459 fails because it returns the "new" value rather than the
"old" one. It's plausible that one of the patches is responsible for
this rather than the underlying code but I don't think so.

This change therefore drops the 'oldlibssh' version check to map to
libssh versions older than 0.9.4, which fixes builds on RHEL-8.

Closes #8548
jay added a commit to jay/curl that referenced this issue Mar 12, 2022
- If building libcurl against an old libssh version missing SSH_S_IFMT
  and SSH_S_IFLNK then use the values from a supported version.

Prior to this change if libssh did not define SSH_S_IFMT and SSH_S_IFLNK
then S_IFMT and S_IFLNK, respectively, were used instead. The problem
with that is the user's S_ stat macros don't have the same values across
platforms. For example Windows has values different from Linux.

Follow-up to 7b0fd39.

Ref: curl#8511 (comment)
Ref: curl#8574

Closes #xxxx
jay added a commit that referenced this issue Mar 14, 2022
- If building libcurl against an old libssh version missing SSH_S_IFMT
  and SSH_S_IFLNK then use the values from a supported version.

Prior to this change if libssh did not define SSH_S_IFMT and SSH_S_IFLNK
then S_IFMT and S_IFLNK, respectively, were used instead. The problem
with that is the user's S_ stat macros don't have the same values across
platforms. For example Windows has values different from Linux.

Follow-up to 7b0fd39.

Ref: #8511 (comment)
Ref: #8574

Closes #8588
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