[PATCH] win32/win32sck.c: dont close() a freed socket os handle #13328
Comments
From @bulk88Created by @bulk88Need an RT number. Perl Info |
From @bulk88Patch attached. It needs a smoke-me test since it needs to be tested on runtime determination alternatives: compile time alternatives: all ioinfo structs from V6 and onward, their beginings are common, msvcr71 and msvcrt are identical, 32 bit ioinfo is (1+1*8)*4=36 bytes long mov ecx, eax msvcr80 msvcr100 and msvcr90 are identical but 80 and 90 come in SxS dll mov ecx, eax |
From @bulk880001-win32-win32sck.c-dont-close-a-freed-socket-os-handle.patchFrom d018f65f9bbe4a9d43864c0848b25e2e7903d882 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Thu, 3 Oct 2013 12:59:13 -0400
Subject: [PATCH] win32/win32sck.c: dont close() a freed socket os handle
This patch is in RT as [perl #120091] is also related to but only partially
fixes [perl #118127].
In #118127 the reporter complained that Microsoft's Application
Verifier was giving 0xC0000008 STATUS_INVALID_HANDLE exceptions for
my_close() with Perl 5.10 inside closesocket(). Although this patch
won't fix any exceptions from non-socket handles being passed
to closesocket(), it will fix any exceptions from calling close()
and internally CloseHandle on a fd for an already closed socket
handle. This makes it easier to use C debuggers on a Win32 Perl in
some cases because of less debugger only bad handle exceptions
(NtGlobalFlag FLG_ENABLE_HANDLE_EXCEPTIONS).
This commit reverts parts of commit 9b1f18150a
"Get rid of PERL_MSVCRT_READFIX" and parts of commit 46e77f1118
"Don't use the PERL_MSVCRT_READFIX when using VC++ 7.x onwards." and
contains additional changes not found in those 2 commits. The method for
selecting the definition of struct ioinfo is flakey (will break if VC >6 is
changed to use the version "6" msvcrt.dll). The assert should catch this
breakage in the future. Alternatives to the ioinfo struct definition
implementation in this patch will be in listed in [perl #120091].
---
win32/win32.h | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
win32/win32sck.c | 12 ++++++----
2 files changed, 76 insertions(+), 4 deletions(-)
diff --git a/win32/win32.h b/win32/win32.h
index 47a1dc6..a1e65fe 100644
--- a/win32/win32.h
+++ b/win32/win32.h
@@ -502,6 +502,74 @@ void win32_wait_for_children(pTHX);
# define PERL_WAIT_FOR_CHILDREN win32_wait_for_children(aTHX)
#endif
+#ifdef PERL_CORE
+/* C doesn't like repeat struct definitions */
+#if defined(__MINGW32__) && (__MINGW32_MAJOR_VERSION>=3)
+#undef _CRTIMP
+#endif
+#ifndef _CRTIMP
+#define _CRTIMP __declspec(dllimport)
+#endif
+
+/*
+ * Control structure for lowio file handles
+ */
+typedef struct {
+ intptr_t osfhnd;/* underlying OS file HANDLE */
+ char osfile; /* attributes of file (e.g., open in text mode?) */
+ char pipech; /* one char buffer for handles opened on pipes */
+ int lockinitflag;
+ CRITICAL_SECTION lock;
+/* this struct defintion breaks ABI compatibility with
+ * not using, cl.exe's native VS version specitfic CRT. */
+#if _MSC_VER >= 1400
+# ifndef _SAFECRT_IMPL
+ /* Not used in the safecrt downlevel. We do not define them, so we cannot
+ * use them accidentally */
+ char textmode : 7;/* __IOINFO_TM_ANSI or __IOINFO_TM_UTF8 or __IOINFO_TM_UTF16LE */
+ char unicode : 1; /* Was the file opened as unicode? */
+ char pipech2[2]; /* 2 more peak ahead chars for UNICODE mode */
+ /* shay's VS 2005 definition conflicts with other public code */
+# if _MSC_VER >= 1500
+ __int64 startpos; /* File position that matches buffer start */
+ BOOL utf8translations; /* Buffer contains translations other than CRLF*/
+ char dbcsBuffer; /* Buffer for the lead byte of dbcs when converting from dbcs to unicode */
+ BOOL dbcsBufferUsed; /* Bool for the lead byte buffer is used or not */
+# endif
+# endif
+#endif
+} ioinfo;
+
+
+/*
+ * Array of arrays of control structures for lowio files.
+ */
+EXTERN_C _CRTIMP ioinfo* __pioinfo[];
+
+/*
+ * Definition of IOINFO_L2E, the log base 2 of the number of elements in each
+ * array of ioinfo structs.
+ */
+#define IOINFO_L2E 5
+
+/*
+ * Definition of IOINFO_ARRAY_ELTS, the number of elements in ioinfo array
+ */
+#define IOINFO_ARRAY_ELTS (1 << IOINFO_L2E)
+
+/*
+ * Access macros for getting at an ioinfo struct and its fields from a
+ * file handle
+ */
+#define _pioinfo(i) (__pioinfo[(i) >> IOINFO_L2E] + ((i) & (IOINFO_ARRAY_ELTS - 1)))
+#define _osfhnd(i) (_pioinfo(i)->osfhnd)
+#define _osfile(i) (_pioinfo(i)->osfile)
+#define _pipech(i) (_pioinfo(i)->pipech)
+
+/* since we are not doing a dup2(), this works fine */
+#define _set_osfhnd(fh, osfh) (void)(_osfhnd(fh) = (intptr_t)osfh)
+#endif
+
/* IO.xs and POSIX.xs define PERLIO_NOT_STDIO to 1 */
#if defined(PERL_EXT_IO) || defined(PERL_EXT_POSIX)
#undef PERLIO_NOT_STDIO
diff --git a/win32/win32sck.c b/win32/win32sck.c
index 5032cf0..fc022a5 100644
--- a/win32/win32sck.c
+++ b/win32/win32sck.c
@@ -663,8 +663,10 @@ int my_close(int fd)
int err;
err = closesocket(osf);
if (err == 0) {
- (void)close(fd); /* handle already closed, ignore error */
- return 0;
+ assert(_osfhnd(fd) == osf); /* catch a bad ioinfo struct def */
+ /* don't close freed handle */
+ _set_osfhnd(fd, INVALID_HANDLE_VALUE);
+ return close(fd);
}
else if (err == SOCKET_ERROR) {
err = get_last_socket_error();
@@ -691,8 +693,10 @@ my_fclose (FILE *pf)
win32_fflush(pf);
err = closesocket(osf);
if (err == 0) {
- (void)fclose(pf); /* handle already closed, ignore error */
- return 0;
+ assert(_osfhnd(win32_fileno(pf)) == osf); /* catch a bad ioinfo struct def */
+ /* don't close freed handle */
+ _set_osfhnd(win32_fileno(pf), INVALID_HANDLE_VALUE);
+ return fclose(pf);
}
else if (err == SOCKET_ERROR) {
err = get_last_socket_error();
--
1.8.0.msysgit.0
|
From @bulk88Another problem I didn't thoroughly think through. There might be a side http://code.ohloh.net/file?fid=wcf_fJvvZwVJPLQrPoTfj7w5Cds&cid=6h85rPuTtus&s=&browser=Default&fp=302894&mpundefined&projSelected=true#L687 but, note the Win32 console handle setting calls. if you read the code, |
From @bulk88On Thu Oct 03 10:08:57 2013, bulk88 wrote:
Bump, can someone look at this patch? -- |
From @steve-m-hayOn 14 October 2013 21:23, bulk88 via RT <perlbug-followup@perl.org> wrote:
I'm not confident that I know enough to judge whether the patch is (*) SDK2003R2 actually fails in t/op/bop.t, crashing perl.exe, but |
|
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88This patch also made #118059 unreproducible for me, the -- |
From @bulk88On Mon Oct 14 15:57:32 2013, shay wrote:
Bump. On IRC jdb a couple weeks ago in #p5p he said didn't have any major issues except worrying about different versions of the struct in different CRTs. jdb also said he assumes TonyC will manage the ticket/smoke/apply the commit, not himself. I'm not pasting the exact words since i'm not jdb. -- |
From @steve-m-hayOn Tue Oct 29 06:32:34 2013, bulk88 wrote:
For my part I'm happy to apply this in the absence of any objections from other quarters, unless Tony has any more comments, in particular regarding its overlap with #118059? It seems to stop the occasional failure in dist/IO/t/cachepropagate-tcp.t for me, which is what #118059 was about. I've already tested with numerous compilers. I don't think the few Win32 smoke testers would add much to what I've already done and it doesn't affect any non-Win32 code. One query, though: what is the comment "shay's VS 2005 definition conflicts with other public code" about in the patch to win32.h?! And as I've just noted on #118127, it doesn't make that one unreproducible for me. I get the same crash with or without this patch. |
From @bulk88On Wed Oct 30 02:15:45 2013, shay wrote:
I think I'll remove mention of #118127 appverifier from the commit message. It not relevant enough to be in git, the RT posts exist anyway for research. Unless you think it should stay in the commit message. I'll also add that it fixes #118059 cachepropagate which previously was never mentioned in the commit message. "shay's VS 2005 definition conflicts with other public code" I'll get an explanation for that later today. Need to do some research. Basically I saw some definitions of that struct in other FOSS code, where __int64 startpos BOOL utf8translations char dbcsBuffer and BOOL dbcsBufferUsed were defined for VC CRT 2005. IDK where you got that struct definition. My REing of assembly says VC 05 shares the same struct as VC 08 and newer, but your struct said VC 05 was shorter VC 08 and newer. I'll recheck the asm later today. -- |
From @steve-m-hayOn 30 October 2013 17:56, bulk88 via RT <perlbug-followup@perl.org> wrote:
Yes, I think mentioning #118059 and not #118127 makes sense.
I assume you're talking about commit 0448a0b which added a bit to VC++ 6.0 SP6: typedef struct { VS .NET 2003 SP1: typedef struct { VS 2005 SP1 + SP1 for Vista: typedef struct { VS 2008 SP1 / VS2010 SP1 / VS2012 Update 3 / VS2013: typedef struct { |
From @bulk88On Wed Oct 30 10:56:49 2013, bulk88 wrote:
Oh no. msvcr80.dll 8.0.50727.3053 uses a 64 byte struct. msvcr80.dll 8.0.50727.42 uses a 40 byte struct. 8.0.50727.42 mov ecx, eax 0x28==40 8.0.50727.3053 mov eax, ecx 2<<6==64 Both of these were in my WinSxS dir. So shay, what is the CRT that your VC 2005 uses? This patch/bug just got alot more complicated. Is the struct size supposed to be runtime discovered by comparing wrapping a fake magic value handle into a CRT fd, then looping over the ioinfo struct and comparing 4/8 byte void *s until they match the magic value handle, then storing the size of the struct in a C static? The static ioinfo struct size is set from PERL_SYS_INIT (the portable version of DllMain in perl)? Or do the above loop search in the miniperl and write it to a .h during the build process, swapping handle to -1 in ioinfo struct happens only in full perl, not miniperl then. Or take a function pointer to a function inside CRT, get DLL handle, then look in DLL's resource section for VS_FIXEDFILEINFO struct and get the file version out of that? Then map the build number of the particular CRT to a hand coded struct size, fatally error on unknown build number? the _msize way? -- |
From @janduboisOn Wed, Oct 30, 2013 at 5:56 PM, bulk88 via RT
Yeah, this is the concern you quoted earlier as: | On IRC jdb a couple weeks ago in #p5p he said didn't have any major issues
To be honest, I don't find any of these approaches reasonable; but the Both the 2nd and 3rd approaches require maintenance of a table of Cheers, |
From @bulk88On Wed Oct 30 16:39:55 2013, shay wrote:
Ok.
I wasn't aware you posted https://rt.perl.org/Ticket/Display.html?id=120091#txn-1266017 before I posted https://rt-archive.perl.org/perl5/Ticket/Display.html?id=120091#txn-1266035 , so my last post was written without seeing your last post (my last post was written while I was offline, and sent it when I got back online). Another VC 2005 CRT I found on my system. 8.0.50727.762 mov ecx, eax 0x38==56 bytes VC service pack numbers can be detected with macro _MSC_FULL_VER which gives a 4 part Windows style version number, so the struct definition could use _MSC_FULL_VER to detect all the different VC 2005 distros. Struct definition after adding VC2005 build 762 typedef struct { This link describes different VC 2005 CRT builds, http://niemiro.co.uk/Blog/windows-update-troubleshooting/visual-c-file-versions/ but doesn't explain ABI compatibility, or which builds are SxS redirection for security fixes of different ABIs. Looking through dir Windows/WinSxS/Policies and .policy files <bindingRedirect oldVersion="8.0.41204.256-8.0.50608.0" newVersion="8.0.50727.42"/> <bindingRedirect oldVersion="8.0.41204.256-8.0.50608.0" newVersion="8.0.50727.762"/> <bindingRedirect oldVersion="8.0.41204.256-8.0.50608.0" newVersion="8.0.50727.3053"/> Below is VC 2005 CRT dll I didn't disassemble, build 6195. <bindingRedirect oldVersion="8.0.41204.256-8.0.50608.0" newVersion="8.0.50727.6195"/> So in MS land, all the VC 2005 CRTs are ABI compatible according to SxS regardless of the VC 2005 linking libs or manifests. I guess the __pioinfo export isn't public API ;) I guess because of SxS, the build number in the manifest is irrelevant to what dll is loaded at runtime. A perl can be built with CRT build 42 from VC 2005 SP0. Executes with build 42 on the builder's PC (the builder never uses Windows Updates). The builder then copies the perl build to another PC, the same perl binary now executes with build 3053 which has a different struct. MS isn't using SxS to have multiple parallel branches of the 2005 CRT, with all the branches simultaneously getting a wave of updates (with each branch getting its own newer higher build number for its particular security update of that branch). Below is made up example of what is NOT HAPPENING with the 2005 CRTs. SP0 KB1 SP1 KB2 KB3 SP2 KB4 KB5 On Wed Oct 30 18:26:53 2013, jdb wrote:
Python uses _msize on the array of ioinfo structs. _pioinfo is a pointer to an array of ioinfo */[]s. Each slice of the 1st array is a pointer to an ioinfo array of 32 ioinfo structs in sequence in memory. The layout is a tiny bit similar to a 2D design of a Perl HV. The _msize is done on the array of 32 ioinfo structs. Since there is always 32 of them, the size in bytes/32 gives the size of 1 ioinfo struct. The other algorithm I am thinking of is, more refined than my earlier descriptions, do _open_osfhandle on some handle to something temporarily (maybe a pipe), find the 32 slice array that the fd falls into (mask operation), search the whole ioinfo array (cast to a HANDLE []) for that handle and use IsBadReadPtr to make sure the search loop doesn't overflow past the end of the ioinfo array, once a 4/8 byte pattern that matches the temp handle is fouind, replace the HANDLE * that has the temp handle with magic value (_open_osfhandle won't return a new fd for a magic value/invalid handle), then do a _get_osfhandle, compare the returned value to the magic value. If _get_osfhandle didn't return the magic value, assume the brute force loop found that some member of ioinfo that just happened to have the same bit pattern as the temp handle. Replace the magic value with the temp handle, keep searching the array for the next instance of temp handle. If access vio mem is found or more than 64(on 32 bit OS)*32 bytes are scanned, croak() or something else (there might be zero interps in the process when PERL_SYS_INIT3 is called, PL_curinterp is NULL). Both options above are forwards compatible if MS adds more members to ioinfo.
Manifest redirection quotes are above. If my SxS interpretation is correct (and I hate SxS with a passion), all msvcr80.dlls are ABI successors to lower build number. Yes, a table will be required but only for VC 2005. The 2005 changes seem to be the biggest problem. The struct never changed in 2008, 2010, or 2012, so its gone stable again. A preprocessor define and a traditional C struct definition of ioinfo struct can deal with everything but VC 2005 builds. The other question if _msize or brute force or VS_VERSIONINFO and a table are used, can <2005 and >2005 be preprocessor hard coded to a ioinfo struct so no startup penalty and 1 C static var is saved? Or does >2005 need to be EOLed by MS first? Or will msvcrt.dll get more members in Win7/8/9? (I dont have any NT 6 machines to get a msvcrt.dll off of them to figure out the size of ioinfo in them) so only VC 2002/2003 can be hard coded safely at this point? -- |
From @janduboisOn Wed, Oct 30, 2013 at 9:11 PM, bulk88 via RT
Personally I would see no problem with requiring either VC6, or VS2008 Just for reference, Python requires VS2010 for building Python 3.3 and Cheers, |
From @steve-m-hay
This sounds like the simplest/sanest option to me.
I wouldn't want to rely on VS2008+ not changing the struct more in the |
From @janduboisOn Thu, Oct 31, 2013 at 11:09 AM, Steve Hay <steve.m.hay@googlemail.com> wrote:
Note though that *all* these approaches always assume that Microsoft Cheers, |
From @bulk88On Thu Oct 31 11:10:22 2013, shay wrote:
Patch revised. Some VCs are hard coded, others use _msize. While developing this patch, I failed the assert in my_close/my_fclose in cpan/Socket/t/Socket.t because I forgot, "ioinfo_size /= IOINFO_ARRAY_ELTS;" in Perl_win32_init. I was surprised that sockets are never tested by anything in "/t" too. So jdb's future compatibility concerns are met if you run a DEBUGGING build with a future unknown CRT with WIN32_DYN_IOINFO_SIZE being on. The assert will fail. The assert also catch jdb's https://rt-archive.perl.org/perl5/Ticket/Display.html?id=120091#txn-1266169 what if ioinfo is changed and the first member isn't "intptr_t osfhnd" anymore. The assert works whether WIN32_DYN_IOINFO_SIZE is on or off. The assert won't fail if the bit pattern at the address where the handle swapper thinks the handle is, happens to be identical to the handle from _get_osfhandle, but I think the chance of that is very remote. After the 2nd or 3rd run through the handle swapper, the assert will fail. I can't image the OS handles being identical to the other members in ioinfo, handle and fd after handle and fd through all of "make test" with DEBUGGING, and not any other CRT I/O calls going haywire from memory corruption because INVALID_HANDLE_VALUE was written over the wrong member in ioinfo. I originally wanted w32_ioinfo_size to be of type "unsigned short". The machine code I saw was nasty. An very long 8 byte long movzx to copy and zero extend/cast 16 bits to pointer size. Then a 2-3 bytes (I dont remember) x86 imul. x86 imul only works on pointer size data 16/32/64bits. So just make the C global pointer size so imul can take it direct as an operand. Doing the offset into the ioinfo array as char math won't work because on 32 bits, ioinfo can be as large as 64 bytes, and there are 32 of them, overflow. Native 16 bit math is impossible on x86-32 because the 16 bit opcodes are 32 bit operators in 32 bit mode. 16 bit math usually synthesized using 32 bit operations then truncation by the compiler, so nothing is saved. Below is the final result "_set_osfhnd(fd, INVALID_HANDLE_VALUE);" with WIN32_DYN_IOINFO_SIZE (I changed the macro defs to enable WIN32_DYN_IOINFO_SIZE on VC 2003 for testing while making this patch). 8B 15 2C 43 0C 28 mov edx, ds:__imp____pioinfo -- |
From @bulk880001-win32-win32sck.c-dont-close-a-freed-socket-os-handle.patchFrom b67319e44d7298e1b9797273bdcc7632404d68ca Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 1 Nov 2013 23:04:35 -0400
Subject: [PATCH] win32/win32sck.c: dont close() a freed socket os handle
This patch is in RT as [perl #120091] but also fixes [perl #118059].
Because the MS C lib, doesn't support sockets natively, Perl uses
open_osfhandle, to wrap a socket into CRT fd. Sockets handles must be
closed with closesocket, not CloseHandle (which CRT close calls).
Undefined behavior will happen otherwise according to MS. Swap the now
closed socket handle in the CRT to INVALID_HANDLE_VALUE. The CRT will
not call CloseHandle on INVALID_HANDLE_VALUE and returns success if it
sees INVALID_HANDLE_VALUE. CloseHandle will never be called on a socket
handle with this patch.
In #118059, a race condition was reported, where accept() failed with
ENOTSOCK, when a psuedofork was done, and connection from the child fake
perl process to parent fake perl process was done. The race condition's
effects occur inside the user mode code in mswsock.dll!_WSPAccept in the
parent perl, which winds up having a kernel handle of an unknown type
in it that is invalid. The invalid handle is passed to kernel calls, which
fail, because the handle is invalid, and the error is translated to
ENOTSOCK. The exact mechanism of the bug and 100% reproducabilty of the
race were never established, since mswsock.dll is closed source.
Another benefit of this patch is making it easier to use C debuggers on
a Win32 Perl because of less debugger-only bad handle exceptions
(NtGlobalFlag FLG_ENABLE_HANDLE_EXCEPTIONS/0xC0000008 STATUS_INVALID_HANDLE).
This commit reverts parts of commit 9b1f18150a
"Get rid of PERL_MSVCRT_READFIX" and parts of commit 46e77f1118
"Don't use the PERL_MSVCRT_READFIX when using VC++ 7.x onwards." and
contains additional changes not found in those 2 commits. The method for
selecting the definition of struct ioinfo isn't perfect. It will break if
VC > 6 is changed to use the older msvcrt.dll. For some versions of the
CRT, like 2005/8.0, it is impossible to know the definition of ioinfo
struct at C compile time, since the struct increased in size a number of
times with higher build numbers of v8.0 CRT. SxS and security updates make
that same perl binary will run with different v8.0 CRTs at different times.
For the case when ioinfo can not be hard coded, introduce
WIN32_DYN_IOINFO_SIZE. With WIN32_DYN_IOINFO_SIZE, the size of the ioinfo
struct is determined on Perl process startup using _mize. Since VC 2013
is a brand new product at the time of this patch, even though its struct
is known, and 2008 through 2012 have been stable so far, don't assume at
this time 2013's ioinfo will be stable. Therefore, VC 2003 and older
(including Mingw's v6.0), 2008 to 2012, are hard coded. 2013 is a candidate
one day to be hard coded. VC 2005 can never be hard coded.
All non-WIN32_DYN_IOINFO_SIZE compilers will work with
WIN32_DYN_IOINFO_SIZE. Non-WIN32_DYN_IOINFO_SIZE mode is simply more
efficient.
For future compatibility concerns, 3 forms of protection are offered.
If __pioinfo isn't exported anymore, the Perl build will break. In
WIN32_DYN_IOINFO_SIZE mode, if __pioinfo isn't heap memory anymore or the
start of a memory block, the runtime panic will happen. In with and without
WIN32_DYN_IOINFO_SIZE, only on a DEBUGGING build, the handle returned by
CRT's _get_osfhandle, which is the authentic handle in the CRT, is compared
to the handle found by accessing the ioinfo struct directly. If they don't
match, the handle swapping code is broken and the assert fails.
---
win32/win32.c | 15 ++++++++
win32/win32.h | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
win32/win32sck.c | 16 +++++++--
3 files changed, 121 insertions(+), 4 deletions(-)
diff --git a/win32/win32.c b/win32/win32.c
index 53962b2..bf91b76 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -160,6 +160,9 @@ static void win32_csighandler(int sig);
START_EXTERN_C
HANDLE w32_perldll_handle = INVALID_HANDLE_VALUE;
char w32_module_name[MAX_PATH+1];
+#ifdef WIN32_DYN_IOINFO_SIZE
+Size_t w32_ioinfo_size;/* avoid 0 extend op b4 mul, otherwise could be a U8 */
+#endif
END_EXTERN_C
static OSVERSIONINFO g_osver = {0, 0, 0, 0, 0, ""};
@@ -4415,6 +4418,18 @@ Perl_win32_init(int *argcp, char ***argvp)
g_osver.dwOSVersionInfoSize = sizeof(g_osver);
GetVersionEx(&g_osver);
+#ifdef WIN32_DYN_IOINFO_SIZE
+ {
+ Size_t ioinfo_size = _msize((void*)__pioinfo[0]);;
+ if((SSize_t)ioinfo_size <= 0) { /* -1 is err */
+ fprintf(stderr, "panic: invalid size for ioinfo\n"); /* no interp */
+ exit(1);
+ }
+ ioinfo_size /= IOINFO_ARRAY_ELTS;
+ w32_ioinfo_size = ioinfo_size;
+ }
+#endif
+
ansify_path();
}
diff --git a/win32/win32.h b/win32/win32.h
index 19dcbf7..a521a41 100644
--- a/win32/win32.h
+++ b/win32/win32.h
@@ -507,6 +507,100 @@ void win32_wait_for_children(pTHX);
# define PERL_WAIT_FOR_CHILDREN win32_wait_for_children(aTHX)
#endif
+#ifdef PERL_CORE
+/* C doesn't like repeat struct definitions */
+#if defined(__MINGW32__) && (__MINGW32_MAJOR_VERSION>=3)
+#undef _CRTIMP
+#endif
+#ifndef _CRTIMP
+#define _CRTIMP __declspec(dllimport)
+#endif
+
+
+/* VV 2005 has multiple ioinfo struct definitions through VC 2005's release life
+ * VC 2008-2012 have been stable but do not assume future VCs will have the
+ * same ioinfo struct, just because past struct stability. If research is done
+ * on the CRTs of future VS, the version check can be bumped up so the newer
+ * VC uses a fixed ioinfo size.
+ */
+#if ! (_MSC_VER < 1400 || (_MSC_VER >= 1500 && _MSC_VER <= 1700) \
+ || defined(__MINGW32__))
+/* size of ioinfo struct is determined at runtime */
+# define WIN32_DYN_IOINFO_SIZE
+#endif
+
+#ifndef WIN32_DYN_IOINFO_SIZE
+/*
+ * Control structure for lowio file handles
+ */
+typedef struct {
+ intptr_t osfhnd;/* underlying OS file HANDLE */
+ char osfile; /* attributes of file (e.g., open in text mode?) */
+ char pipech; /* one char buffer for handles opened on pipes */
+ int lockinitflag;
+ CRITICAL_SECTION lock;
+/* this struct defintion breaks ABI compatibility with
+ * not using, cl.exe's native VS version specitfic CRT. */
+# if _MSC_VER >= 1400 && _MSC_VER < 1500
+# error "This ioinfo struct is incomplete for Visual C 2005"
+# endif
+/* VC 2005 CRT has atleast 3 different definitions of this struct based on the
+ * CRT DLL's build number. */
+# if _MSC_VER >= 1500
+# ifndef _SAFECRT_IMPL
+ /* Not used in the safecrt downlevel. We do not define them, so we cannot
+ * use them accidentally */
+ char textmode : 7;/* __IOINFO_TM_ANSI or __IOINFO_TM_UTF8 or __IOINFO_TM_UTF16LE */
+ char unicode : 1; /* Was the file opened as unicode? */
+ char pipech2[2]; /* 2 more peak ahead chars for UNICODE mode */
+ __int64 startpos; /* File position that matches buffer start */
+ BOOL utf8translations; /* Buffer contains translations other than CRLF*/
+ char dbcsBuffer; /* Buffer for the lead byte of dbcs when converting from dbcs to unicode */
+ BOOL dbcsBufferUsed; /* Bool for the lead byte buffer is used or not */
+# endif
+# endif
+} ioinfo;
+#else
+typedef intptr_t ioinfo;
+#endif
+
+/*
+ * Array of arrays of control structures for lowio files.
+ */
+EXTERN_C _CRTIMP ioinfo* __pioinfo[];
+
+/*
+ * Definition of IOINFO_L2E, the log base 2 of the number of elements in each
+ * array of ioinfo structs.
+ */
+#define IOINFO_L2E 5
+
+/*
+ * Definition of IOINFO_ARRAY_ELTS, the number of elements in ioinfo array
+ */
+#define IOINFO_ARRAY_ELTS (1 << IOINFO_L2E)
+
+/*
+ * Access macros for getting at an ioinfo struct and its fields from a
+ * file handle
+ */
+#ifdef WIN32_DYN_IOINFO_SIZE
+# define _pioinfo(i) ((intptr_t *) \
+ (((Size_t)__pioinfo[(i) >> IOINFO_L2E])/* * to head of array ioinfo [] */\
+ /* offset to the head of a particular ioinfo struct */ \
+ + (((i) & (IOINFO_ARRAY_ELTS - 1)) * w32_ioinfo_size)) \
+ )
+/* first slice of ioinfo is always the OS handle */
+# define _osfhnd(i) (*(_pioinfo(i)))
+#else
+# define _pioinfo(i) (__pioinfo[(i) >> IOINFO_L2E] + ((i) & (IOINFO_ARRAY_ELTS - 1)))
+# define _osfhnd(i) (_pioinfo(i)->osfhnd)
+#endif
+
+/* since we are not doing a dup2(), this works fine */
+# define _set_osfhnd(fh, osfh) (void)(_osfhnd(fh) = (intptr_t)osfh)
+#endif /* PERL_CORE */
+
/* IO.xs and POSIX.xs define PERLIO_NOT_STDIO to 1 */
#if defined(PERL_EXT_IO) || defined(PERL_EXT_POSIX)
#undef PERLIO_NOT_STDIO
diff --git a/win32/win32sck.c b/win32/win32sck.c
index 5f98910..674add2 100644
--- a/win32/win32sck.c
+++ b/win32/win32sck.c
@@ -54,6 +54,10 @@ static struct servent* win32_savecopyservent(struct servent*d,
static int wsock_started = 0;
+#ifdef WIN32_DYN_IOINFO_SIZE
+EXTERN_C Size_t w32_ioinfo_size;
+#endif
+
EXTERN_C void
EndSockets(void)
{
@@ -689,8 +693,10 @@ int my_close(int fd)
int err;
err = closesocket(osf);
if (err == 0) {
- (void)close(fd); /* handle already closed, ignore error */
- return 0;
+ assert(_osfhnd(fd) == osf); /* catch a bad ioinfo struct def */
+ /* don't close freed handle */
+ _set_osfhnd(fd, INVALID_HANDLE_VALUE);
+ return close(fd);
}
else if (err == SOCKET_ERROR) {
err = get_last_socket_error();
@@ -717,8 +723,10 @@ my_fclose (FILE *pf)
win32_fflush(pf);
err = closesocket(osf);
if (err == 0) {
- (void)fclose(pf); /* handle already closed, ignore error */
- return 0;
+ assert(_osfhnd(win32_fileno(pf)) == osf); /* catch a bad ioinfo struct def */
+ /* don't close freed handle */
+ _set_osfhnd(win32_fileno(pf), INVALID_HANDLE_VALUE);
+ return fclose(pf);
}
else if (err == SOCKET_ERROR) {
err = get_last_socket_error();
--
1.7.9.msysgit.0
|
From @steve-m-hayOn 2 November 2013 03:40, bulk88 via RT <perlbug-followup@perl.org> wrote:
Thanks, applied in commit b47a847 after testing with: VC++ 6.0 SP6 x86 |
From @steve-m-hayPatch now applied, so closing ticket. |
|
@steve-m-hay - Status changed from 'open' to 'resolved' |
From @greergaOn Wed, 30 Oct 2013, Jan Dubois wrote:
I guess I better watch commits because VC2005 is what my Windows 2000 -- |
From @bulk88On Mon Nov 04 17:35:21 2013, perl@greerga.m-l.org wrote:
My day to day C compiler is VC 2003. My more rarely used alternate is VC 2008. -- |
From @bulk88While doing archeology, I stumbled onto something that should be documented for this ticket for history reasons. I never realized it when I wrote this patch/ticket, but problem that this ticket fixed was actually fixed in Oct 2000 by gsar at http://perl5.git.perl.org/perl.git/commitdiff/a10b7b7eee64efea010bfdba91243503341ba68d . It was removed in http://perl5.git.perl.org/perl.git/commit/9b1f18150a in Dec 2010 by jdb but I didn't see the win32sck.c changes at the end of the diff, I thought http://perl5.git.perl.org/perl.git/commit/9b1f18150a simply removed the infrastructure for modifying/accessing CRT fd structs which existed for other, validly removed reasons, and didnt realize it removed the sockets fix (swap kernel handle in fd struct before closing the socket fd) I was (now) re-adding. rjbs commented on IRC can this be tested for? not really, it fixed a intermittent test fail race condition in dist\IO\t\cachepropagate-tcp.t from #118059, but I see no way of recreating the race condition on demand. TLDR, not happening without a kernel driver (see htrace feature of WinDbg http://blogs.msdn.com/b/spatdsg/archive/2005/03/23/401020.aspx ) and stepping threads Recreating the race would mean trying to synchronize step by step asm wise the user mode Microsoft closed source OS sockets library in 2 different threads/psuedo-forks. Basically something in that sockets library, when you call "closesocket" it lets the other sockets library using threads know the socket handle is dead in user mode or "half dead state" and the other threads's sockets behavior works correctly. If you call closesocket then close/CloseHandle, something is probably screwed up during the cleanup, and there is a race condition as the other thread sees a half freed socket and tries to use it. Or the socket reverts from half dead (locked) to invalid (unlocked) to freed (unlockable) and during invalid state the user mode sockets library okays doing things on the socket, but the kernel side tcp driver doesn't recognize the handle anymore due to the CloseHandle and somewhere in there is a race in all that. I know the win32 Sockets library will do callbacks from kernel mode into user mode to update statuses/events of sockets, which are user mode structs that get synced with kernel mode structs. Written to a process's VM space from kernel-mode is a bit complicated (threads, map and lock a user-mode page into kernel vm, SEGVs, locking paging, locking data bus, etc), so running a user mode callback is easiest solution for MS's devs. -- |
Migrated from rt.perl.org#120091 (status was 'resolved')
Searchable as RT120091$
The text was updated successfully, but these errors were encountered: