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

Deprecated API usage in system_win32.c #4995

Closed
FuccDucc opened this issue Feb 28, 2020 · 8 comments
Closed

Deprecated API usage in system_win32.c #4995

FuccDucc opened this issue Feb 28, 2020 · 8 comments
Labels
needs-info not-a-bug This is not a bug in curl Windows Windows-specific

Comments

@FuccDucc
Copy link

The current version of cURL is using a deprecated Windows API function GetVersionEx at this location: system_win32.c @ 226

Is this really neccesary? For increased reliability, refactoring it using the replacement, Version Helper Functions, sounds like a good idea.

@jay jay added the Windows Windows-specific label Feb 28, 2020
@jay
Copy link
Member

jay commented Feb 28, 2020

VerifyVersionInfo is used when available. I don't see the problem.

/cc @captain-caveman2k

@bagder bagder added the not-a-bug This is not a bug in curl label Feb 28, 2020
@FuccDucc
Copy link
Author

FuccDucc commented Feb 28, 2020

VerifyVersionInfo is used when available. I don't see the problem.

Well, let me reverse the question.

This usage causes a compiler warning in projects that use cURL.

Why would you keep using a deprecated function? Why would you use it when a replacement (or alternative) is available, when using the deprecated function is also unnecesary, since all old Windows versions support the replacement. Without a reason not to upgrade it. Pro's versus cons.

Deprecated functions, like the MS article for this one also says, also may not be reliable (or unavailable) in future and thus there is a risk they'll behave in unexpected ways. This future risk is unnecesary, since you can replace the usage.

@jay
Copy link
Member

jay commented Feb 28, 2020

This usage causes a compiler warning in projects that use cURL.

What is the warning, how are you building curl and what are you using to build it?

Why would you keep using a deprecated function?

Because not all build systems or targets may support the replacement function.

@FuccDucc
Copy link
Author

FuccDucc commented Feb 28, 2020

What is the warning, how are you building curl and what are you using to build it?

All modern versions of Visual Studio, for example, give a deprecation warning.

Because not all build systems or targets may support the replacement function.

Yes they do, GetVersionEx is a Windows API (talking about targets) and the replacement, like I said, supports all old versions of Windows.

Since it appears no one understands anything, maybe it's better to go and read this article: https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getversionexa

It is deprecated. The following description matters to understand the pro's and cons (as I said before, you don't want to risk unexpected behavior in the future for no good reason right? Any arguing against upgrading is just a case of principle/project practise, so I will leave this issue now and you can do whatever you want):

[GetVersionEx may be altered or unavailable for releases after Windows 8.1. Instead, use the Version Helper functions]
With the release of Windows 8.1, the behavior of the GetVersionEx API has changed in the value it will return for the operating system version

@jay
Copy link
Member

jay commented Feb 28, 2020

VerifyVersionInfo should be used when available. Please tell us:

What is the warning, how are you building curl and what are you using to build it?

@bagder
Copy link
Member

bagder commented Feb 28, 2020

Since it appears no one understands anything

I think it is you who misses the point: the code tries to use the right function already and we'd appreciate if you helped us figure out why it doesn't!

@captain-caveman2k
Copy link
Contributor

captain-caveman2k commented Feb 29, 2020

I've only just seen this - so my apologies for jumping in late.

The idea with #if block at line 218 is that GetVersionEx() should only be used on Non-Windows NT based versions of Windows (i.e. Windows 9x / ME and versions of NT earlier than Windows 2000, ie Windows NT 3.5x and 4.0) where a) it isn't deprecated and b) VerifyVersionInfo() isn't available. Anything newer should be using VerifyVersionInfo() as seen at line 345.

It would seem that for some reason this #if block isn't working as intended and I, like my colleagues, would like to try and understand why.

  • Where was the source code/libcurl API obtained from (Official tarball/zip, Github, NuGet, etc...)?
  • Are you using libcurl with the default config-win32.h?
  • Are you compiling libcurl yourself? If so how is it built (Visual Studio Projects, nmake, cmake, msys)?

@bagder
Copy link
Member

bagder commented Mar 5, 2020

No feedback, closing.

@bagder bagder closed this as completed Mar 5, 2020
jay added a commit to jay/curl that referenced this issue Mar 7, 2020
- Replace _WIN32_WINNT_WIN2K symbol with 0x500 since not all build
  systems have the symbol.

Prior to this change if a build system did not have _WIN32_WINNT_WIN2K
then GetVersionEx was unintentionally used instead of VerifyVersionInfo.
The former is a deprecated function intended to be used only if the OS
targeted is earlier than Windows 2000, and build systems may warn of its
use.

Reported-by: FuccDucc@users.noreply.github.com

Fixes curl#4995
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Mar 8, 2020
.. because not all Windows build systems have those symbols, and even
those that do may be missing newer symbols (eg the Windows 7 SDK does
not define _WIN32_WINNT_WIN10).

Those symbols are used in build-time logic to decide which API to use
and prior to this change if the symbols were missing it would have
resulted in deprecated API being used when more recent functions were
available (eg GetVersionEx used instead of VerifyVersionInfo).

Reported-by: FuccDucc@users.noreply.github.com

Probably fixes curl#4995
Closes curl#5057
jay added a commit that referenced this issue Mar 21, 2020
.. because not all Windows build systems have those symbols, and even
those that do may be missing newer symbols (eg the Windows 7 SDK does
not define _WIN32_WINNT_WIN10).

Those symbols are used in build-time logic to decide which API to use
and prior to this change if the symbols were missing it would have
resulted in deprecated API being used when more recent functions were
available (eg GetVersionEx used instead of VerifyVersionInfo).

Reported-by: FuccDucc@users.noreply.github.com

Probably fixes #4995
Closes #5057
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-info not-a-bug This is not a bug in curl Windows Windows-specific
Development

Successfully merging a pull request may close this issue.

4 participants