Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
On Oct 24 08:58, Takashi Yano wrote:
> Hi Corinna,
>
> On Fri, 22 Oct 2021 17:11:13 +0200
> Corinna Vinschen wrote:
> > Just to close this up prior to the 3.3.0 release...
> >
> > Given we never actually strived for 32<->64 bit interoperability, it's
> > hard to argue why this should be different for the clipboard stuff.
> >
> > Running 32 and 64 bit Cygwin versions in parallel doesn't actually make
> > much sense for most people anyway, unless they explicitely develop for
> > 32 and 64 bit systems under Cygwin. From a productivity point of view
> > there's no good reason to run more than one arch.
> >
> > So I agree with Ken here. It's probably not worth the trouble.
>
> Current code below in fhandler_clipboard.cc causes access violation
> if clipboard is accessed between 32 and 64 bit cygwin.
>
> cygcb_t *clipbuf = (cygcb_t *) cb_data;
>
> if (pos < (off_t) clipbuf->len)
> {
> ret = ((len > (clipbuf->len - pos)) ? (clipbuf->len - pos) : len);
> memcpy (ptr, clipbuf->data + pos , ret);
> pos += ret;
> }
>
> Don't you think this should be fixed?
Well, if you put it this way...
I didn't get that it's crashing, so yeah, in that case a fix would
be nice, of course.
Corinna
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
Hi Corinna,
On Fri, 22 Oct 2021 17:11:13 +0200
Corinna Vinschen wrote:
> Just to close this up prior to the 3.3.0 release...
>
> Given we never actually strived for 32<->64 bit interoperability, it's
> hard to argue why this should be different for the clipboard stuff.
>
> Running 32 and 64 bit Cygwin versions in parallel doesn't actually make
> much sense for most people anyway, unless they explicitely develop for
> 32 and 64 bit systems under Cygwin. From a productivity point of view
> there's no good reason to run more than one arch.
>
> So I agree with Ken here. It's probably not worth the trouble.
Current code below in fhandler_clipboard.cc causes access violation
if clipboard is accessed between 32 and 64 bit cygwin.
cygcb_t *clipbuf = (cygcb_t *) cb_data;
if (pos < (off_t) clipbuf->len)
{
ret = ((len > (clipbuf->len - pos)) ? (clipbuf->len - pos) : len);
memcpy (ptr, clipbuf->data + pos , ret);
pos += ret;
}
Don't you think this should be fixed?
--
Takashi Yano
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
Ken Brown wrote: On 10/23/2021 1:35 AM, Mark Geisert wrote: Corinna Vinschen wrote: Just to close this up prior to the 3.3.0 release... Given we never actually strived for 32<->64 bit interoperability, it's hard to argue why this should be different for the clipboard stuff. Running 32 and 64 bit Cygwin versions in parallel doesn't actually make much sense for most people anyway, unless they explicitely develop for 32 and 64 bit systems under Cygwin. From a productivity point of view there's no good reason to run more than one arch. So I agree with Ken here. It's probably not worth the trouble. Sorry, I've been sidetracked for a bit. I can agree with Ken too. The only circumstance I could think of where multiple internal format support might be useful (to non-developers) was some user hanging on to an older Cygwin because it was needed to support something else (s/w or h/w) old and non-upgradeable. Doesn't seem very likely at this point. I'll try to get the v2 patch out over this weekend. Same end-result for same environments as the v1 patch, but incorporating all the comments I received. I think Corinna was saying that the whole idea of making the 32-bit and 64-bit clipboards interoperable is not worth the trouble. Oh, I didn't read it that way. But that works for me too. Color it dropped :-). Thanks, ..mark
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
On 10/23/2021 1:35 AM, Mark Geisert wrote: Corinna Vinschen wrote: Just to close this up prior to the 3.3.0 release... Given we never actually strived for 32<->64 bit interoperability, it's hard to argue why this should be different for the clipboard stuff. Running 32 and 64 bit Cygwin versions in parallel doesn't actually make much sense for most people anyway, unless they explicitely develop for 32 and 64 bit systems under Cygwin. From a productivity point of view there's no good reason to run more than one arch. So I agree with Ken here. It's probably not worth the trouble. Sorry, I've been sidetracked for a bit. I can agree with Ken too. The only circumstance I could think of where multiple internal format support might be useful (to non-developers) was some user hanging on to an older Cygwin because it was needed to support something else (s/w or h/w) old and non-upgradeable. Doesn't seem very likely at this point. I'll try to get the v2 patch out over this weekend. Same end-result for same environments as the v1 patch, but incorporating all the comments I received. I think Corinna was saying that the whole idea of making the 32-bit and 64-bit clipboards interoperable is not worth the trouble. To that end, does Jon's suggestion of /usr/include/sys/cygwin.h seem like the best location to define struct cygcb_t for use by both Cygwin and cygutils package? Thanks much, ..mark
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
Hi all, Corinna Vinschen wrote: On Oct 11 08:11, Ken Brown wrote: On 10/11/2021 2:13 AM, Mark Geisert wrote: It's just that after submitting the patch I realized that, if we really are going to support both Cygwin archs (x86_64 and i686), there is still the issue of different cygcb_t layouts between Cygwin versions being ignored. Specifically, the fhandler_clipboard::fstat routine can't tell which Cygwin environment has set the clipboard contents. My original patch takes care of 32-bit and 64-bit, providing both are running Cygwin >= 3.3.0 (presumably). What if it was a different version (pre 3.3.0) that set the contents? I wonder if this is worth the trouble. Right now we have a problem in which data written to /dev/clipboard in one arch can't be read in the other arch. The fix will appear in Cygwin 3.3.0. Do we really have to try to make the fix apply retroactively in case the user updates one arch but not the other? Just to close this up prior to the 3.3.0 release... Given we never actually strived for 32<->64 bit interoperability, it's hard to argue why this should be different for the clipboard stuff. Running 32 and 64 bit Cygwin versions in parallel doesn't actually make much sense for most people anyway, unless they explicitely develop for 32 and 64 bit systems under Cygwin. From a productivity point of view there's no good reason to run more than one arch. So I agree with Ken here. It's probably not worth the trouble. Sorry, I've been sidetracked for a bit. I can agree with Ken too. The only circumstance I could think of where multiple internal format support might be useful (to non-developers) was some user hanging on to an older Cygwin because it was needed to support something else (s/w or h/w) old and non-upgradeable. Doesn't seem very likely at this point. I'll try to get the v2 patch out over this weekend. Same end-result for same environments as the v1 patch, but incorporating all the comments I received. To that end, does Jon's suggestion of /usr/include/sys/cygwin.h seem like the best location to define struct cygcb_t for use by both Cygwin and cygutils package? Thanks much, ..mark
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
On Oct 11 08:11, Ken Brown wrote: > On 10/11/2021 2:13 AM, Mark Geisert wrote: > > It's just that after submitting the patch I realized that, if we really > > are going to support both Cygwin archs (x86_64 and i686), there is still > > the issue of different cygcb_t layouts between Cygwin versions being > > ignored. > > > > Specifically, the fhandler_clipboard::fstat routine can't tell which > > Cygwin environment has set the clipboard contents. My original patch > > takes care of 32-bit and 64-bit, providing both are running Cygwin >= > > 3.3.0 (presumably). What if it was a different version (pre 3.3.0) that > > set the contents? > > I wonder if this is worth the trouble. Right now we have a problem in which > data written to /dev/clipboard in one arch can't be read in the other arch. > The fix will appear in Cygwin 3.3.0. Do we really have to try to make the > fix apply retroactively in case the user updates one arch but not the other? Just to close this up prior to the 3.3.0 release... Given we never actually strived for 32<->64 bit interoperability, it's hard to argue why this should be different for the clipboard stuff. Running 32 and 64 bit Cygwin versions in parallel doesn't actually make much sense for most people anyway, unless they explicitely develop for 32 and 64 bit systems under Cygwin. From a productivity point of view there's no good reason to run more than one arch. So I agree with Ken here. It's probably not worth the trouble. Corinna
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
On 10/11/2021 2:13 AM, Mark Geisert wrote: It's just that after submitting the patch I realized that, if we really are going to support both Cygwin archs (x86_64 and i686), there is still the issue of different cygcb_t layouts between Cygwin versions being ignored. Specifically, the fhandler_clipboard::fstat routine can't tell which Cygwin environment has set the clipboard contents. My original patch takes care of 32-bit and 64-bit, providing both are running Cygwin >= 3.3.0 (presumably). What if it was a different version (pre 3.3.0) that set the contents? I wonder if this is worth the trouble. Right now we have a problem in which data written to /dev/clipboard in one arch can't be read in the other arch. The fix will appear in Cygwin 3.3.0. Do we really have to try to make the fix apply retroactively in case the user updates one arch but not the other? Ken
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
Ken Brown wrote: On 10/9/2021 10:29 AM, Jon Turney wrote: [...] On 10/8/2021 5:52 AM, Takashi Yano wrote: [...] Thanks all for the comments; much appreciated. They'll be factored into v2 in one form or another. I pronounced my original patch "bad" not because of any problem with code operation or struct cygcb_t definition. I used anonymous union and anonymous struct internally to mostly realize what Takashi suggested for layout, just naming the items cb_* rather than tv_* or other. The code works as intended, 32- and 64-bit. It's just that after submitting the patch I realized that, if we really are going to support both Cygwin archs (x86_64 and i686), there is still the issue of different cygcb_t layouts between Cygwin versions being ignored. Specifically, the fhandler_clipboard::fstat routine can't tell which Cygwin environment has set the clipboard contents. My original patch takes care of 32-bit and 64-bit, providing both are running Cygwin >= 3.3.0 (presumably). What if it was a different version (pre 3.3.0) that set the contents? So I'm working on a heuristic to identify which cygcb_t layout is present in the clipboard data. This will hopefully distinguish between the 3 historical cygcb_t layouts as well as x86_64 differing from i686 for each one. Stay tuned, ..mark
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
On 10/9/2021 10:29 AM, Jon Turney wrote:
On 07/10/2021 06:22, Mark Geisert wrote:
> The cygutils package has two programs, putclip and getclip, that also
> depend on the layout of the cygcb_t. At present they have duplicate
> defs of struct cygcb_t defined here as no Cygwin header provides it.
This struct should maybe be in sys/cygwin.h or similar, if it's expected to be
used in user-space as well.
Good idea.
On 09/10/2021 15:19, Ken Brown wrote:
On 10/8/2021 5:52 AM, Takashi Yano wrote:
How about simply just:
diff --git a/winsup/cygwin/fhandler_clipboard.cc
b/winsup/cygwin/fhandler_clipboard.cc
index ccdb295f3..d822f4fc4 100644
--- a/winsup/cygwin/fhandler_clipboard.cc
+++ b/winsup/cygwin/fhandler_clipboard.cc
@@ -28,9 +28,10 @@ static const WCHAR *CYGWIN_NATIVE =
L"CYGWIN_NATIVE_CLIPBOARD";
typedef struct
{
- timestruc_t timestamp;
- size_t len;
- char data[1];
+ uint64_t tv_sec;
+ uint64_t tv_nsec;
+ uint64_t len;
+ char data[1];
} cygcb_t;
The only problem with this is that it might leave readers scratching their
heads unless they look at the commit that introduced this. What
I think the solution to that is a "comment" like "we don't use 'struct timespec'
here as it's different size on different arches and that causes problem XYZ".
And the same for size_t.
Ken
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
On 07/10/2021 06:22, Mark Geisert wrote:
> The cygutils package has two programs, putclip and getclip, that also
> depend on the layout of the cygcb_t. At present they have duplicate
> defs of struct cygcb_t defined here as no Cygwin header provides it.
This struct should maybe be in sys/cygwin.h or similar, if it's expected
to be used in user-space as well.
On 09/10/2021 15:19, Ken Brown wrote:
On 10/8/2021 5:52 AM, Takashi Yano wrote:
How about simply just:
diff --git a/winsup/cygwin/fhandler_clipboard.cc
b/winsup/cygwin/fhandler_clipboard.cc
index ccdb295f3..d822f4fc4 100644
--- a/winsup/cygwin/fhandler_clipboard.cc
+++ b/winsup/cygwin/fhandler_clipboard.cc
@@ -28,9 +28,10 @@ static const WCHAR *CYGWIN_NATIVE =
L"CYGWIN_NATIVE_CLIPBOARD";
typedef struct
{
- timestruc_t timestamp;
- size_t len;
- char data[1];
+ uint64_t tv_sec;
+ uint64_t tv_nsec;
+ uint64_t len;
+ char data[1];
} cygcb_t;
The only problem with this is that it might leave readers scratching
their heads unless they look at the commit that introduced this. What
I think the solution to that is a "comment" like "we don't use 'struct
timespec' here as it's different size on different arches and that
causes problem XYZ".
(I think that's preferable to conditional code which we assert (but
don't ensure) is the same on all arches)
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
On 10/8/2021 5:52 AM, Takashi Yano wrote:
How about simply just:
diff --git a/winsup/cygwin/fhandler_clipboard.cc
b/winsup/cygwin/fhandler_clipboard.cc
index ccdb295f3..d822f4fc4 100644
--- a/winsup/cygwin/fhandler_clipboard.cc
+++ b/winsup/cygwin/fhandler_clipboard.cc
@@ -28,9 +28,10 @@ static const WCHAR *CYGWIN_NATIVE =
L"CYGWIN_NATIVE_CLIPBOARD";
typedef struct
{
- timestruc_t timestamp;
- size_t len;
- char data[1];
+ uint64_t tv_sec;
+ uint64_t tv_nsec;
+ uint64_t len;
+ char data[1];
} cygcb_t;
The only problem with this is that it might leave readers scratching their heads
unless they look at the commit that introduced this. What about something like
the following, in which the code speaks for itself:
diff --git a/winsup/cygwin/fhandler_clipboard.cc
b/winsup/cygwin/fhandler_clipboard.cc
index ccdb295f3..028c00f1e 100644
--- a/winsup/cygwin/fhandler_clipboard.cc
+++ b/winsup/cygwin/fhandler_clipboard.cc
@@ -26,12 +26,26 @@ details. */
static const WCHAR *CYGWIN_NATIVE = L"CYGWIN_NATIVE_CLIPBOARD";
+#ifdef __x86_64__
typedef struct
{
timestruc_t timestamp;
size_t len;
char data[1];
} cygcb_t;
+#else
+/* Use same layout. */
+typedef struct
+{
+ struct
+ {
+int64_t tv_sec;
+int64_t tv_nsec;
+ }timestamp;
+ uint64_t len;
+ char data[1];
+} cygcb_t;
+#endif
fhandler_dev_clipboard::fhandler_dev_clipboard ()
: fhandler_base (), pos (0), membuffer (NULL), msize (0)
@@ -74,7 +88,14 @@ fhandler_dev_clipboard::set_clipboard (const void *buf,
size_t len)
}
clipbuf = (cygcb_t *) GlobalLock (hmem);
+#ifdef __x86_64__
clock_gettime (CLOCK_REALTIME, &clipbuf->timestamp);
+#else
+ timestruc_t ts;
+ clock_gettime (CLOCK_REALTIME, &ts);
+ clipbuf->timestamp->tv_sec = ts.tv_sec;
+ clipbuf->timestamp->tv_nsec = ts.tv_nsec;
+#endif
clipbuf->len = len;
memcpy (clipbuf->data, buf, len);
@@ -179,7 +200,14 @@ fhandler_dev_clipboard::fstat (struct stat *buf)
&& (hglb = GetClipboardData (format))
&& (clipbuf = (cygcb_t *) GlobalLock (hglb)))
{
+#ifdef __x86_64__
buf->st_atim = buf->st_mtim = clipbuf->timestamp;
+#else
+ timestruc_t ts;
+ ts.tv_sec = clipbuf->timestamp->tv_sec;
+ ts.tv_nsec = clipbuf->timestamp->tv_nsec;
+ buf->st_atim = buf->st_mtim = ts;
+#endif
buf->st_size = clipbuf->len;
GlobalUnlock (hglb);
}
Ken
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
How about simply just:
diff --git a/winsup/cygwin/fhandler_clipboard.cc
b/winsup/cygwin/fhandler_clipboard.cc
index ccdb295f3..d822f4fc4 100644
--- a/winsup/cygwin/fhandler_clipboard.cc
+++ b/winsup/cygwin/fhandler_clipboard.cc
@@ -28,9 +28,10 @@ static const WCHAR *CYGWIN_NATIVE =
L"CYGWIN_NATIVE_CLIPBOARD";
typedef struct
{
- timestruc_t timestamp;
- size_t len;
- char data[1];
+ uint64_t tv_sec;
+ uint64_t tv_nsec;
+ uint64_t len;
+ char data[1];
} cygcb_t;
fhandler_dev_clipboard::fhandler_dev_clipboard ()
@@ -74,7 +75,10 @@ fhandler_dev_clipboard::set_clipboard (const void *buf,
size_t len)
}
clipbuf = (cygcb_t *) GlobalLock (hmem);
- clock_gettime (CLOCK_REALTIME, &clipbuf->timestamp);
+ struct timespec ts;
+ clock_gettime (CLOCK_REALTIME, &ts);
+ clipbuf->tv_sec = ts.tv_sec;
+ clipbuf->tv_nsec = ts.tv_nsec;
clipbuf->len = len;
memcpy (clipbuf->data, buf, len);
@@ -179,7 +183,10 @@ fhandler_dev_clipboard::fstat (struct stat *buf)
&& (hglb = GetClipboardData (format))
&& (clipbuf = (cygcb_t *) GlobalLock (hglb)))
{
- buf->st_atim = buf->st_mtim = clipbuf->timestamp;
+ struct timespec ts;
+ ts.tv_sec = clipbuf->tv_sec;
+ ts.tv_nsec = clipbuf->tv_nsec;
+ buf->st_atim = buf->st_mtim = ts;
buf->st_size = clipbuf->len;
GlobalUnlock (hglb);
}
--
Takashi Yano
Re: [PATCH] Cygwin: Make native clipboard layout same for 32- and 64-bit
Mark Geisert wrote: This allows correct copy and pasting between the two Cygwin flavors. [...] Eh, that's a bad patch. Don't apply. It's OK for concept review. v2 forthcoming. Thanks, ..mark
