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

Fix Segfault on Windows 64 bits #1623

Merged
merged 1 commit into from May 18, 2018

Conversation

@mlasson
Copy link
Contributor

mlasson commented Feb 20, 2018

The segfault happens when you use a wildcard '*' or '?'
in the argument of the caml program compiled on Windows
64bits.

The handle was corrupted because it is cast from
intptr_t (64bits) to int (32bits).

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

@nojb

This comment has been minimized.

Copy link
Contributor

nojb commented Feb 20, 2018

@mlasson

This comment has been minimized.

Copy link
Contributor Author

mlasson commented Feb 20, 2018

@nojb: ok, also it seems that my change break the compilation on the old compiler, I should probably add the #ifdef too

@shindere ok ! I will add a non-regression test and a change entry tomorrow

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Feb 20, 2018

#ifdef _MSC_VER
#ifndef _UINTPTR_T_DEFINED
#ifdef _WIN64
typedef unsigned long int uintptr_t;

This comment has been minimized.

@nojb

nojb Feb 21, 2018

Contributor

This is wrong; long is 32-bit on Windows, see https://msdn.microsoft.com/en-us/library/3b2e7499.aspx.

#define _UINTPTR_T_DEFINED
#endif
#ifndef _INTPTR_T_DEFINED
#ifdef _WIN64
typedef long int intptr_t;

This comment has been minimized.

@nojb

nojb Feb 21, 2018

Contributor

Same problem as above.

@nojb

This comment has been minimized.

Copy link
Contributor

nojb commented Feb 21, 2018

More generally, are we sure that the fallback for when intptr_t is not defined is still necessary? Can this be the case for the supported versions of Windows? @dra27?

@mlasson mlasson force-pushed the mlasson:trunk branch from c2b50fd to 2b433e0 Feb 21, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Feb 21, 2018

@nojb - the fallback is for old compilers, rather than old versions of Windows. It's becoming less likely to be hit, but I think you'd be surprised how many MSVC users are still tied to old versions for annoying reasons (I, for one, still have to use VS2003 for a project - though not an OCaml project)

@mlasson

This comment has been minimized.

Copy link
Contributor Author

mlasson commented Feb 21, 2018

@dra27 : do you think it makes sense to keep my commit 1b35d7b ?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Feb 27, 2018

Assigning to @dra27 to make a decision about the need to support versions of msvc without intptr_t.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Feb 27, 2018

Out of curiosity, which type was returned by _wfindfirst on older msvc without intptr_t?

@alainfrisch alainfrisch added the bug label Feb 28, 2018

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

The fallback definitions of intptr_t and uintptr_t can be simplified.

#if defined(_MSC_VER) && !defined(_UINTPTR_T_DEFINED)
#ifdef _MSC_VER
# ifndef _UINTPTR_T_DEFINED
#ifdef _WIN64

This comment has been minimized.

@xavierleroy

xavierleroy Mar 2, 2018

Contributor

Just use uintnat and intnat defined in caml/config.h, these integer types are guaranteed to have the same size as pointer types:

typedef uintnat uintptr_t;
#define _UINTPTR_T_DEFINED
#endif
#ifndef _INTPTR_T_DEFINED
#ifdef _WIN64

This comment has been minimized.

@xavierleroy

xavierleroy Mar 2, 2018

Contributor

Likewise, replace this #ifdef _WIN64...#endif by

typedef intnat intptr_t;

@mlasson mlasson force-pushed the mlasson:trunk branch from 2b433e0 to 315af55 Mar 2, 2018

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

Looks good to me. @dra27 @alainfrisch : would you like to have another look at this code, or can we merge as is?

@nojb

nojb approved these changes Mar 5, 2018

#define _UINTPTR_T_DEFINED
#endif
#ifndef _INTPTR_T_DEFINED
typedef intnat intptr_t;

This comment has been minimized.

@alainfrisch

alainfrisch Mar 6, 2018

Contributor

We still have the following in channels.c:

#if defined(_MSC_VER) && !defined(_INTPTR_T_DEFINED)
typedef int intptr_t;
#define _INTPTR_T_DEFINED
#endif
....
...
int win_CRT_fd_of_filedescr(value handle)
{
  if (CRT_fd_val(handle) != NO_CRT_FD) {
    return CRT_fd_val(handle);
  } else {
    int fd = _open_osfhandle((intptr_t) Handle_val(handle), O_BINARY);
    if (fd == -1) uerror("channel_of_descr", Nothing);
    CRT_fd_val(handle) = fd;
    return fd;
  }
}

Wouldn't that cause a similar problem, on 64-bit and old enough versions of MSVC? (Or perhaps all 64-bit versions do define intptr_t?)

This comment has been minimized.

@nojb

nojb Mar 6, 2018

Contributor

The typedef is wrong, but in principle it is safe: file HANDLEs in Win 64 are still 32-bits. See https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%29.aspx.

This comment has been minimized.

@alainfrisch

alainfrisch Mar 6, 2018

Contributor

Ok. Perhaps worth fixing it, but can be done independently of this PR (unless we prefer to add the missing typedef in a central place).

This comment has been minimized.

@mlasson

mlasson Mar 6, 2018

Author Contributor

What about adding after the definition intnat and uintnat in config.h the following piece of code:

#ifndef _UINTPTR_T_DEFINED
typedef uintnat uintptr_t;
#define _UINTPTR_T_DEFINED
#endif
#ifndef _INTPTR_T_DEFINED
typedef intnat intptr_t;
#define _INTPTR_T_DEFINED
#endif

and remove all other typedef of intptr_t in the codebase ?

This comment has been minimized.

@mlasson

mlasson Mar 8, 2018

Author Contributor

Or could simply use "intnat" and "uintnat" in place of intptr_t ?
(The only drawbacks I can think of is the risk to forget to replace it in future code).

This comment has been minimized.

@xavierleroy

xavierleroy Mar 8, 2018

Contributor

Or could simply use "intnat" and "uintnat" in place of intptr_t ?

That might be more prudent than defining {u,}intptr_t yourself, with the risk that some system header file doesn't understand what you did and tries to define it again.

On the other hand, if all recent enough versions of MSVC define those standard types, maybe we should just give up on supporting the older versions.

This comment has been minimized.

@alainfrisch

alainfrisch Mar 8, 2018

Contributor

I fully agree. So, we need to decide between:

  • Use uintnat/intnat (unconditionally)
  • Use intptr_t/uintptr_t, which means no longer supporting old versions of MSVC

My understanding is that versions of MSVC that don't support intptr_t are also versions that don't come with a 64-bit variant, meaning they are really old. Support could be restored purely in the build system by passing something like /Dintptr_t=int to cl.exe, if really needed.

@dra27 : you would be the (likely only) one to care for such old versions, so your input would be much appreciated!

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Mar 8, 2018

Sorry - I should have checked this more closely more quickly, as there's been far too much thought invested in maintaining VC6 support here. Regarding channels.c, the typedef was intentionally simplified because no 64-bit headers are missing the definition, so it will always be 32-bit.

It's highly unlikely anyone's managed to write code which typedef's uintptr_t and intptr_t without also defining the guards and not noticed their error - they're in crtdefs.h which is included by everything. However, in principle I agree with @xavierleroy's caution.

I personally, even in C, prefer types to be being cast to the correct type, but I don't have a rational objection to using intnat or uintnat instead of intptr_t and uintptr_t, as long as it remains internal to the library/runtime.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 9, 2018

@dra27 @alainfrisch Could you two come to a consensus as to how to proceed?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Apr 9, 2018

I think the consensus converges towards using uintnat/intnat (unconditionally) on each call site.

@nojb

This comment has been minimized.

Copy link
Contributor

nojb commented Apr 10, 2018

@mlasson could you rebase this PR, replacing all uses of (u)intptr_t with (u)intnat and remove all corresponding #define's so we can merge this? Thanks!

@mlasson

This comment has been minimized.

Copy link
Contributor Author

mlasson commented Apr 11, 2018

Yes, I will do that today !

@mlasson mlasson force-pushed the mlasson:trunk branch from 315af55 to cebd202 Apr 11, 2018

@mlasson

This comment has been minimized.

Copy link
Contributor Author

mlasson commented Apr 11, 2018

Ok. It fails to compile on 32bits (it does compile on 64 bits).

The type equality: "uintptr_t = uintnat" is wrong on msvc 32bits... because uinptr_t expand to "unsigned int" (as define in vs headers) and uintnat to "unsigned long" (as defined in config.h). Note that both are 32 bits unsigned integer but they are not "equal" wrt. typechecking.

So what about putting the polyfill macro (#if defined(_MSC_VER) && !defined(_UINTPTR_T_DEFINED) ...) in m.h (the one for win32 only), use (u)intptr_t when we need it ?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented May 3, 2018

So what about putting the polyfill macro (#if defined(_MSC_VER) && !defined(_UINTPTR_T_DEFINED) ...) in m.h (the one for win32 only), use (u)intptr_t when we need it ?

This would work only if m.h included is included after the standard header that normally defines those types, no?

I'm leaning towards just using (u)intptr_t, which means that users who need to compile with very old versions of MSVC will need to do something (patch the code, or pass -Dintptr_t=... to the C compiler).

At some point, we will want to require newer versions of MSVC anyway to benefit from better C99 support.

@dra27 : do you agree with this approach?

@mlasson mlasson force-pushed the mlasson:trunk branch from cebd202 to ec5d2b1 May 16, 2018

@mlasson

This comment has been minimized.

Copy link
Contributor Author

mlasson commented May 16, 2018

I implemented @alainfrisch's suggestion. I putted the "polyfill" in m-nt.h / s-nt.h (I also added "HAS_STDINT_H" for MSC_VER >= 1600 when it was introduced).

@nojb
Copy link
Contributor

nojb left a comment

Have some questions about the new version of the patch; I am not sure will work as intended in the present form.

#if defined(_MSC_VER) && _MSC_VER < 1600
#ifndef _UINTPTR_T_DEFINED
#ifdef _WIN64
typedef __int64 intptr_t;

This comment has been minimized.

@nojb

nojb May 16, 2018

Contributor

uintptr_t

#ifdef _WIN64
typedef __int64 intptr_t;
#else
typedef int intptr_t;

This comment has been minimized.

@nojb

nojb May 16, 2018

Contributor

idem

#define _INTPTR_T_DEFINED
#endif /* _INTPTR_T_DEFINED */
#else
#define HAS_STDINT_H

This comment has been minimized.

@nojb

nojb May 16, 2018

Contributor

I don't understand:

  • m.h is included before any other header (see caml/config.h), so _UINTPTR_T_DEFINED will not be defined here even if they are defined later (in stdint.h I guess).
  • Also, by defining HAS_STDINT_H, you are saying it is OK to include stdint.h, but you are defining it precisely in the case of old compilers, where it supposedly does not exist, agree?
  • Why guard both against old compilers and undefined intptr_t? It seems to me that it would be enough to guard against one of those conditions...
@@ -17,7 +17,7 @@

#define OCAML_OS_TYPE "Win32"

#ifdef __MINGW32__
#if defined(__MINGW32__) || (defined(_MSC_VER) && _MSC_VER >= 1600)

This comment has been minimized.

@nojb

nojb May 16, 2018

Contributor

Is this change related to this PR?

This comment has been minimized.

@mlasson

mlasson May 16, 2018

Author Contributor

The idea is : either we use stdint.h, or we do the typedefs ourselves.

@mlasson mlasson force-pushed the mlasson:trunk branch 2 times, most recently from 44173f4 to 462db3f May 16, 2018

@damiendoligez damiendoligez added this to the 4.07 milestone May 16, 2018

@dra27
Copy link
Contributor

dra27 left a comment

Looks good to me apart from a minor nit and an agreement with @nojb's comment about removing the double-guard in m-nt.h (if the guard is to remain, then it should be academically correct - 1200, not 1600).

@@ -17,7 +17,7 @@

#define OCAML_OS_TYPE "Win32"

#ifdef __MINGW32__
#if defined(__MINGW32__) || (defined(_MSC_VER) && _MSC_VER >= 1600)

This comment has been minimized.

@dra27

dra27 May 16, 2018

Contributor

This can just be #if defined(__MINGW32__) || _MSC_VER >= 1600 - even in MS world, undefined macros are expanded to 0 in comparisons.

@@ -63,3 +63,22 @@
#endif

#define FLAT_FLOAT_ARRAY

#if defined(_MSC_VER) && _MSC_VER < 1600

This comment has been minimized.

@dra27

dra27 May 16, 2018

Contributor

This guard is unnecessary (and it would be 1200, not 1600: Visual Studio 2010 added stdint.h, but these typedefs existed since Visual Studio .NET 2002 in, amongst others, stdarg.h and stddef.h)

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented May 16, 2018

@alainfrisch - to answer your questions (finally!). We do include OS headers before caml/config.h - apart anything else, some of the headers depend upon it, I think. For your interest in history, _wfindfirst returned long in Visual C++ 6 (i.e. before intptr_t was added).

@nojb

This comment has been minimized.

Copy link
Contributor

nojb commented May 16, 2018

@mlasson could you address @dra27's comments, rebase to remove the useless intermediate commits and add @dra27, @alainfrisch and myself to the list of reviewers so that we can move to merge this? Thanks!

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented May 16, 2018

Just to throw one final spanner into these works. Just to be clear, the only Microsoft C Compiler which doesn't have definitions anywhere for uintptr_t and intptr_t and which is still just capable of building OCaml (or at least was in 2015 when I had too much time on my hands while performing some Messiahs...) is Visual C++ 6.0 (1998). Visual Studio .NET 2002 onwards have these - they're just in stddef.h prior to Visual Studio 2010. I would have no problem with altering the logic to define HAS_STDINT_H for Visual Studio 2010 and later (as this PR already does) but instead #include <stddef.h> for VS 2008 and earlier. i.e. in config.h have:

#ifdef HAS_STDINT_H
#include <stdint.h>
#elif defined(_MSC_VER)
#include <stddef.h>
#endif

All the stuff in m-nt.h could then be dropped...

@mlasson

This comment has been minimized.

Copy link
Contributor Author

mlasson commented May 16, 2018

Thanks @dra27 I like the last solution.

The only thing that I would prefer to avoid is to pollute "config.h" with a _MSC_VER macro.

What do you (or anyone else) think about one of these solutions:
0) include stddef.h in m-nt.h if version < 1200

  1. always include stddef.h in m-nt.h
  2. define "HAS_STDDEF_H" in s-nt.h and include stddef.h when present,
  3. always include stddef.h in config.h
    ?
@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented May 16, 2018

There are gcc-specific things in config.h already, so having an MS-specific one is not pollution 😉

However, as stddef.h is a standard header, I can't envisage any harm in always including it in config.h (option 3) - could @xavierleroy (or another dev) quickly sign off on that too, and I think we're then good to go?

@mlasson mlasson force-pushed the mlasson:trunk branch from 462db3f to 075b933 May 17, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented May 17, 2018

could @xavierleroy (or another dev) quickly sign off on that too, and I think we're then good to go?

If stddef.h is both part of C99 and available on MSVC, then yes let's include it.

@nojb

This comment has been minimized.

Copy link
Contributor

nojb commented May 18, 2018

If stddef.h is both part of C99 and available on MSVC, then yes let's include it.

Yes, it is; so if @dra27 provides his formal approval, I will merge by the end of the day unless someone speaks up.

@@ -17,7 +17,7 @@

#define OCAML_OS_TYPE "Win32"

#ifdef __MINGW32__
#if defined(__MINGW32__) || _MSC_VER >= 1200

This comment has been minimized.

@dra27

dra27 May 18, 2018

Contributor

This should be 1600.

stdint.h is only available from VS 2010 - the 1200 was to do with the _UINTPTR_T_DEFINED guards (since for _MSC_VER < 1200, stddef.h doesn't include the intptr_t types)

This comment has been minimized.

@mlasson

mlasson May 18, 2018

Author Contributor

Ok, thanks ! It seems we are converging.

Fix Segfault on Windows 64 bits
The segfault happens when you use a wildcard '*' or '?'
in the argument of the caml program compiled on Windows
64bits.

The handle was corrupted because is cast from
intptr_t (64bits) to int (32bits).

In order to make sure that both `intptr_t` and `uintptr_t`
are available on MSVC we forced the inclusion of <stdint.h>
for versions that have it (starting from Visual Studio 2010
(MSVC version 1600)). For older versions, the typedefs may be
found in <stddef.h>; therefore we forced the inclusion of this
standard header in config.h.

@mlasson mlasson force-pushed the mlasson:trunk branch from 075b933 to 576620a May 18, 2018

@dra27

dra27 approved these changes May 18, 2018

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented May 18, 2018

Great, thank you - for the sake of completeness, I'll allow the CI to complete before merging.

@dra27 dra27 merged commit 02e93d6 into ocaml:trunk May 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented May 18, 2018

Cherry-picked to 4.07 in 33f576b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.