Re: [patch/cygwin] Remove dependency on __COPY_CONTEXT_SIZE

[Corinna Vinschen <vinschen at redhat dot com>]

On Mar 31 17:30, Pedro Alves wrote:
> On 03/31/2015 04:42 PM, Corinna Vinschen wrote:
> > On Mar 31 15:58, Pedro Alves wrote:
> >> On 03/31/2015 03:36 PM, Corinna Vinschen wrote:
> >>> On Mar 31 13:34, Pedro Alves wrote:
> >>>> On 03/30/2015 11:04 AM, Corinna Vinschen wrote:
> >>>>
> >>>>> @@ -820,7 +819,7 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
> >>>>>  #endif
> >>>>>  	warning (("%s"), s);
> >>>>>      }
> >>>>> -#ifdef __COPY_CONTEXT_SIZE
> >>>>> +#ifdef __CYGWIN__
> >>>>>    else
> >>>>>      {
> >>>>>        /* Got a cygwin signal marker.  A cygwin signal is followed by
> >>>>> @@ -847,8 +846,8 @@ handle_output_debug_string (struct target_waitstatus *ourstatus)
> >>>>>  	  else if ((x = (LPCVOID) (uintptr_t) strtoull (p, NULL, 0))
> >>>>>  		   && ReadProcessMemory (current_process_handle, x,
> >>>>>  					 &saved_context,
> >>>>> -					 __COPY_CONTEXT_SIZE, &n)
> >>>>> -		   && n == __COPY_CONTEXT_SIZE)
> >>>>> +					 sizeof (CONTEXT), &n)
> >>>>
> >>>> Is that really wise?  AFAIK, the size of the CONTEXT structure can
> >>>> grow as MSFT adds more registers to support newer machines.
> >>>
> >>> No, that's not possible.  The CONTEXT structure matches the platform.
> >>> It doesn't even contain a version number.  Consider that the structure
> >>> is available in user space.  If Microsoft changes the size on a given
> >>> platform, applications built for this platform might crash due to
> >>> overwritten memory.  They wouldn't do that.
> >>
> >> That's not true.   GetThreadContext takes a size parameter,
> >> and only writes to the bits that the caller requests with
> >> context.ContextFlags.
> > 
> > The ContextFlags member is not a size parameter,
> 
> I didn't say it was.  The GetThreadContext function takes
> an IN+OUT size parameter in _addition to the ContextFlags flag.

Uhm...no, it doesn't.  The prototype is

  BOOL WINAPI GetThreadContext(
    _In_     HANDLE hThread,
    _Inout_  LPCONTEXT lpContext
  );

Only the ContextFlags member qualifies what's written to *lpContext.  See
https://msdn.microsoft.com/en-us/library/windows/desktop/ms679362%28v=vs.85%29.aspx

> > Right, but this does not change the size of the CONTEXT datatype.  The
> > additional AVX values require more space than available in the CONTEXT
> > struct.  That's why using CONTEXT_XSTATE and the AVX functions require
> > to use InitializeContext; the size required to get these values is
> > larger than CONTEXT, thus the function returns ERROR_INSUFFICIENT_BUFFER
> > if ContextLength is == sizeof (CONTEXT) only.
> 
> I'm almost sure in the old days, the CONTEXT structure didn't have
> the ExtendedRegisters field at all.

I don't know about that.  But the fact that CONTEXT is not a opaque
structure but exposed to user space speaks against that.  Every time
a Win32 datatype needs a change, MSFT kept the old datatype intact
and added an "Ex" or "2" type instead, just as with the functions.

> I think it's bad to hard code
> the size of the CONTEXT structure, but won't argue further.  Patch
> is OK if you'd really like to apply it as is.

Thanks.  But incidentally I retract the patch.  It seems we made a
mistake both, on 32 and 64 bit Cygwin as far as the definition of
__COPY_CONTEXT_SIZE is concerned.  Changing that to sizeof(CONTEXT) now
would potentially break backward compatibility with all Cygwin versions
up to today.  Oh well.

Sorry for the longish discussion for nothing :(


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Attachment: pgpMgGhPz8vE8.pgp
Description: PGP signature