[PATCH v2] Cygwin: clipboard: Fix a bug in read().

[PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Takashi Yano]

- Fix a bug in fhandler_dev_clipboard::read() that the second read
  fails with 'Bad address'.

Addresses:
  https://cygwin.com/pipermail/cygwin/2021-December/250141.html
---
 winsup/cygwin/fhandler_clipboard.cc | 2 +-
 winsup/cygwin/release/3.3.4         | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 winsup/cygwin/release/3.3.4

diff --git a/winsup/cygwin/fhandler_clipboard.cc b/winsup/cygwin/fhandler_clipboard.cc
index 0b87dd352..ae10228a7 100644
--- a/winsup/cygwin/fhandler_clipboard.cc
+++ b/winsup/cygwin/fhandler_clipboard.cc
@@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& len)
       if (pos < (off_t) clipbuf->cb_size)
 	{
 	  ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - pos : len;
-	  memcpy (ptr, &clipbuf[1] + pos , ret);
+	  memcpy (ptr, (char *) &clipbuf[1] + pos, ret);
 	  pos += ret;
 	}
     }
diff --git a/winsup/cygwin/release/3.3.4 b/winsup/cygwin/release/3.3.4
new file mode 100644
index 000000000..f1c32a1a5
--- /dev/null
+++ b/winsup/cygwin/release/3.3.4
@@ -0,0 +1,6 @@
+Bug Fixes
+---------
+
+- Fix a bug in fhandler_dev_clipboard::read() that the second read
+  fails with 'Bad address'.
+  Addresses: https://cygwin.com/pipermail/cygwin/2021-December/250141.html
-- 
2.34.1

Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Corinna Vinschen]

On Dec  7 23:00, Takashi Yano wrote:
> - Fix a bug in fhandler_dev_clipboard::read() that the second read
>   fails with 'Bad address'.
> 
> Addresses:
>   https://cygwin.com/pipermail/cygwin/2021-December/250141.html
> ---
>  winsup/cygwin/fhandler_clipboard.cc | 2 +-
>  winsup/cygwin/release/3.3.4         | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>  create mode 100644 winsup/cygwin/release/3.3.4
> 
> diff --git a/winsup/cygwin/fhandler_clipboard.cc b/winsup/cygwin/fhandler_clipboard.cc
> index 0b87dd352..ae10228a7 100644
> --- a/winsup/cygwin/fhandler_clipboard.cc
> +++ b/winsup/cygwin/fhandler_clipboard.cc
> @@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& len)
>        if (pos < (off_t) clipbuf->cb_size)
>  	{
>  	  ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - pos : len;
> -	  memcpy (ptr, &clipbuf[1] + pos , ret);
> +	  memcpy (ptr, (char *) &clipbuf[1] + pos, ret);

I'm always cringing a bit when I see this kind of expression. Personally
I think (ptr + offset) is easier to read than &ptr[offset], but of course
that's just me.  If you agree, would it be ok to change the above to

  (char *) (clipbuf + 1)

while you're at it?  If you like the ampersand expression more, it's ok,
too, of course.  Please push.


Thanks,
Corinna

Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Thomas Wolff]

Am 07.12.2021 um 15:23 schrieb Corinna Vinschen:
> On Dec  7 23:00, Takashi Yano wrote:
>> - Fix a bug in fhandler_dev_clipboard::read() that the second read
>>    fails with 'Bad address'.
>>
>> Addresses:
>>    https://cygwin.com/pipermail/cygwin/2021-December/250141.html
>> ---
>>   winsup/cygwin/fhandler_clipboard.cc | 2 +-
>>   winsup/cygwin/release/3.3.4         | 6 ++++++
>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>   create mode 100644 winsup/cygwin/release/3.3.4
>>
>> diff --git a/winsup/cygwin/fhandler_clipboard.cc b/winsup/cygwin/fhandler_clipboard.cc
>> index 0b87dd352..ae10228a7 100644
>> --- a/winsup/cygwin/fhandler_clipboard.cc
>> +++ b/winsup/cygwin/fhandler_clipboard.cc
>> @@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& len)
>>         if (pos < (off_t) clipbuf->cb_size)
>>   	{
>>   	  ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - pos : len;
>> -	  memcpy (ptr, &clipbuf[1] + pos , ret);
>> +	  memcpy (ptr, (char *) &clipbuf[1] + pos, ret);
> I'm always cringing a bit when I see this kind of expression. Personally
> I think (ptr + offset) is easier to read than &ptr[offset], but of course
> that's just me.  If you agree, would it be ok to change the above to
>
>    (char *) (clipbuf + 1)
>
> while you're at it?  If you like the ampersand expression more, it's ok,
> too, of course.  Please push.
In this specific case I think it's actually more confusing because of 
the type mangling that's intended in the clipbuf.
At quick glance, it looks a bit as if the following were meant:

   (char *) clipbuf + 1


I'd even make it clearer like

+	  memcpy (ptr, ((char *) &clipbuf[1]) + pos, ret);
or even
+	  memcpy (ptr, ((char *) (&clipbuf[1])) + pos, ret);

Thomas

Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Brian Inglis]

On 2021-12-07 13:18, Thomas Wolff wrote:
> 
> Am 07.12.2021 um 15:23 schrieb Corinna Vinschen:
>> On Dec  7 23:00, Takashi Yano wrote:
>>> - Fix a bug in fhandler_dev_clipboard::read() that the second read
>>>    fails with 'Bad address'.
>>>
>>> Addresses:
>>>    https://cygwin.com/pipermail/cygwin/2021-December/250141.html
>>> ---
>>>   winsup/cygwin/fhandler_clipboard.cc | 2 +-
>>>   winsup/cygwin/release/3.3.4         | 6 ++++++
>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>   create mode 100644 winsup/cygwin/release/3.3.4
>>>
>>> diff --git a/winsup/cygwin/fhandler_clipboard.cc 
>>> b/winsup/cygwin/fhandler_clipboard.cc
>>> index 0b87dd352..ae10228a7 100644
>>> --- a/winsup/cygwin/fhandler_clipboard.cc
>>> +++ b/winsup/cygwin/fhandler_clipboard.cc
>>> @@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& 
>>> len)
>>>         if (pos < (off_t) clipbuf->cb_size)
>>>       {
>>>         ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - 
>>> pos : len;
>>> -      memcpy (ptr, &clipbuf[1] + pos , ret);
>>> +      memcpy (ptr, (char *) &clipbuf[1] + pos, ret);

>> I'm always cringing a bit when I see this kind of expression. Personally
>> I think (ptr + offset) is easier to read than &ptr[offset], but of course
>> that's just me.  If you agree, would it be ok to change the above to
>>
>>    (char *) (clipbuf + 1)
>>
>> while you're at it?  If you like the ampersand expression more, it's ok,
>> too, of course.  Please push.

> In this specific case I think it's actually more confusing because of 
> the type mangling that's intended in the clipbuf.
> At quick glance, it looks a bit as if the following were meant:
> 
>    (char *) clipbuf + 1
> 
> I'd even make it clearer like
> 
> +      memcpy (ptr, ((char *) &clipbuf[1]) + pos, ret);
> or even
> +      memcpy (ptr, ((char *) (&clipbuf[1])) + pos, ret);

If the intent is to address:

	clipbuf + pos + 1

use either that or:

	&clipbuf[pos + 1]

to avoid obscuring the intent,
and add comments to make it clearer!

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
[Data in binary units and prefixes, physical quantities in SI.]

Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Mark Geisert]

Brian Inglis wrote:
> On 2021-12-07 13:18, Thomas Wolff wrote:
>>
>> Am 07.12.2021 um 15:23 schrieb Corinna Vinschen:
>>> On Dec  7 23:00, Takashi Yano wrote:
>>>> - Fix a bug in fhandler_dev_clipboard::read() that the second read
>>>>    fails with 'Bad address'.
>>>>
>>>> Addresses:
>>>>    https://cygwin.com/pipermail/cygwin/2021-December/250141.html
>>>> ---
>>>>   winsup/cygwin/fhandler_clipboard.cc | 2 +-
>>>>   winsup/cygwin/release/3.3.4         | 6 ++++++
>>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>>   create mode 100644 winsup/cygwin/release/3.3.4
>>>>
>>>> diff --git a/winsup/cygwin/fhandler_clipboard.cc 
>>>> b/winsup/cygwin/fhandler_clipboard.cc
>>>> index 0b87dd352..ae10228a7 100644
>>>> --- a/winsup/cygwin/fhandler_clipboard.cc
>>>> +++ b/winsup/cygwin/fhandler_clipboard.cc
>>>> @@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& len)
>>>>         if (pos < (off_t) clipbuf->cb_size)
>>>>       {
>>>>         ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - pos : len;
>>>> -      memcpy (ptr, &clipbuf[1] + pos , ret);
>>>> +      memcpy (ptr, (char *) &clipbuf[1] + pos, ret);
> 
>>> I'm always cringing a bit when I see this kind of expression. Personally
>>> I think (ptr + offset) is easier to read than &ptr[offset], but of course
>>> that's just me.  If you agree, would it be ok to change the above to
>>>
>>>    (char *) (clipbuf + 1)
>>>
>>> while you're at it?  If you like the ampersand expression more, it's ok,
>>> too, of course.  Please push.
> 
>> In this specific case I think it's actually more confusing because of the type 
>> mangling that's intended in the clipbuf.
>> At quick glance, it looks a bit as if the following were meant:
>>
>>    (char *) clipbuf + 1
>>
>> I'd even make it clearer like
>>
>> +      memcpy (ptr, ((char *) &clipbuf[1]) + pos, ret);
>> or even
>> +      memcpy (ptr, ((char *) (&clipbuf[1])) + pos, ret);
> 
> If the intent is to address:
> 
>      clipbuf + pos + 1
> 
> use either that or:
> 
>      &clipbuf[pos + 1]
> 
> to avoid obscuring the intent,
> and add comments to make it clearer!

Boy am I kicking myself for screwing up the original here and opening this can of 
worms.  Brian, you'd be correct if clipbuf was (char *) like anything-buf often 
is.  But here it's a struct defining the initial part of a dynamic char buffer.

So my original
     &clipbuf[1]
to mean "just after the defining struct" was OK.  But the code needed a ptr to 
some char offset after that and
     &clipbuf[1] + pos
was wrong.  Casting the left term to (char *) would fix it.  But I like Corinna's 
choice of
     (char *) (clipbuf + 1)
to be most elegant and clear of all.  Now enclose that in parens and append the 
char offset so the new expression is
     ((char *) (clipbuf + 1)) + pos
and all should be golden.  I don't think extra commentary is needed in code.

(I think.)

..mark

Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Brian Inglis]

On 2021-12-07 23:30, Mark Geisert wrote:
> Brian Inglis wrote:
>> On 2021-12-07 13:18, Thomas Wolff wrote:
>>> Am 07.12.2021 um 15:23 schrieb Corinna Vinschen:
>>>> On Dec  7 23:00, Takashi Yano wrote:
>>>>> - Fix a bug in fhandler_dev_clipboard::read() that the second read
>>>>>    fails with 'Bad address'.
>>>>>
>>>>> Addresses:
>>>>>    https://cygwin.com/pipermail/cygwin/2021-December/250141.html
>>>>> ---
>>>>>   winsup/cygwin/fhandler_clipboard.cc | 2 +-
>>>>>   winsup/cygwin/release/3.3.4         | 6 ++++++
>>>>>   2 files changed, 7 insertions(+), 1 deletion(-)
>>>>>   create mode 100644 winsup/cygwin/release/3.3.4
>>>>>
>>>>> diff --git a/winsup/cygwin/fhandler_clipboard.cc 
>>>>> b/winsup/cygwin/fhandler_clipboard.cc
>>>>> index 0b87dd352..ae10228a7 100644
>>>>> --- a/winsup/cygwin/fhandler_clipboard.cc
>>>>> +++ b/winsup/cygwin/fhandler_clipboard.cc
>>>>> @@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, 
>>>>> size_t& len)
>>>>>         if (pos < (off_t) clipbuf->cb_size)
>>>>>       {
>>>>>         ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - 
>>>>> pos : len;
>>>>> -      memcpy (ptr, &clipbuf[1] + pos , ret);
>>>>> +      memcpy (ptr, (char *) &clipbuf[1] + pos, ret);

>>>> I'm always cringing a bit when I see this kind of expression. 
>>>> Personally I think (ptr + offset) is easier to read than 
>>>> &ptr[offset], but of course that's just me. If you agree, would
>>>> it be ok to change the above to
>>>>
>>>>    (char *) (clipbuf + 1)
>>>>
>>>> while you're at it?  If you like the ampersand expression more,
>>>> it's ok, too, of course. Please push.

>>> In this specific case I think it's actually more confusing because of 
>>> the type mangling that's intended in the clipbuf.
>>> At quick glance, it looks a bit as if the following were meant:
>>>
>>>    (char *) clipbuf + 1
>>>
>>> I'd even make it clearer like
>>>
>>> +      memcpy (ptr, ((char *) &clipbuf[1]) + pos, ret);
>>> or even
>>> +      memcpy (ptr, ((char *) (&clipbuf[1])) + pos, ret);

>> If the intent is to address:
>>
>>      clipbuf + pos + 1
>>
>> use either that or:
>>
>>      &clipbuf[pos + 1]
>>
>> to avoid obscuring the intent,
>> and add comments to make it clearer!

> Boy am I kicking myself for screwing up the original here and opening 
> this can of worms.  Brian, you'd be correct if clipbuf was (char *) like 
> anything-buf often is.  But here it's a struct defining the initial part 
> of a dynamic char buffer.
> 
> So my original
>      &clipbuf[1]
> to mean "just after the defining struct" was OK.  But the code needed a 
> ptr to some char offset after that and
>      &clipbuf[1] + pos
> was wrong.  Casting the left term to (char *) would fix it.  But I like 
> Corinna's choice of
>      (char *) (clipbuf + 1)
> to be most elegant and clear of all.  Now enclose that in parens and 
> append the char offset so the new expression is
>      ((char *) (clipbuf + 1)) + pos

In this context, (char *)clipbuf + sizeof clipbuf would actually be 
clearer. Or hide the expression in a cb_text(clipbuf[,pos?]) macro.

You could make the format and intent clear and obvious by appending 
cb_text[0|1|...] (some C++ compatible form) into cygcb_t in 
sys/clipboard.h.

> and all should be golden.  I don't think extra commentary is needed
> in code. (I think.)
Where else put commentary?
In this case it should be mandatory, as this is an address hack to 
access the clipboard text, and there were questions about that expression.

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
[Data in binary units and prefixes, physical quantities in SI.]

Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Takashi Yano]

On Tue, 7 Dec 2021 22:30:44 -0800
Mark Geisert wrote:
> Brian Inglis wrote:
> > On 2021-12-07 13:18, Thomas Wolff wrote:
> >>
> >> Am 07.12.2021 um 15:23 schrieb Corinna Vinschen:
> >>> On Dec  7 23:00, Takashi Yano wrote:
> >>>> - Fix a bug in fhandler_dev_clipboard::read() that the second read
> >>>>    fails with 'Bad address'.
> >>>>
> >>>> Addresses:
> >>>>    https://cygwin.com/pipermail/cygwin/2021-December/250141.html
> >>>> ---
> >>>>   winsup/cygwin/fhandler_clipboard.cc | 2 +-
> >>>>   winsup/cygwin/release/3.3.4         | 6 ++++++
> >>>>   2 files changed, 7 insertions(+), 1 deletion(-)
> >>>>   create mode 100644 winsup/cygwin/release/3.3.4
> >>>>
> >>>> diff --git a/winsup/cygwin/fhandler_clipboard.cc 
> >>>> b/winsup/cygwin/fhandler_clipboard.cc
> >>>> index 0b87dd352..ae10228a7 100644
> >>>> --- a/winsup/cygwin/fhandler_clipboard.cc
> >>>> +++ b/winsup/cygwin/fhandler_clipboard.cc
> >>>> @@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& len)
> >>>>         if (pos < (off_t) clipbuf->cb_size)
> >>>>       {
> >>>>         ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - pos : len;
> >>>> -      memcpy (ptr, &clipbuf[1] + pos , ret);
> >>>> +      memcpy (ptr, (char *) &clipbuf[1] + pos, ret);
> > 
> >>> I'm always cringing a bit when I see this kind of expression. Personally
> >>> I think (ptr + offset) is easier to read than &ptr[offset], but of course
> >>> that's just me.  If you agree, would it be ok to change the above to
> >>>
> >>>    (char *) (clipbuf + 1)
> >>>
> >>> while you're at it?  If you like the ampersand expression more, it's ok,
> >>> too, of course.  Please push.
> > 
> >> In this specific case I think it's actually more confusing because of the type 
> >> mangling that's intended in the clipbuf.
> >> At quick glance, it looks a bit as if the following were meant:
> >>
> >>    (char *) clipbuf + 1
> >>
> >> I'd even make it clearer like
> >>
> >> +      memcpy (ptr, ((char *) &clipbuf[1]) + pos, ret);
> >> or even
> >> +      memcpy (ptr, ((char *) (&clipbuf[1])) + pos, ret);
> > 
> > If the intent is to address:
> > 
> >      clipbuf + pos + 1
> > 
> > use either that or:
> > 
> >      &clipbuf[pos + 1]
> > 
> > to avoid obscuring the intent,
> > and add comments to make it clearer!
> 
> Boy am I kicking myself for screwing up the original here and opening this can of 
> worms.  Brian, you'd be correct if clipbuf was (char *) like anything-buf often 
> is.  But here it's a struct defining the initial part of a dynamic char buffer.
> 
> So my original
>      &clipbuf[1]
> to mean "just after the defining struct" was OK.  But the code needed a ptr to 
> some char offset after that and
>      &clipbuf[1] + pos
> was wrong.  Casting the left term to (char *) would fix it.  But I like Corinna's 
> choice of
>      (char *) (clipbuf + 1)
> to be most elegant and clear of all.  Now enclose that in parens and append the 
> char offset so the new expression is
>      ((char *) (clipbuf + 1)) + pos
> and all should be golden.  I don't think extra commentary is needed in code.
> 
> (I think.)

I think the following patch makes the intent clearer.
What do you think?


From d0aee9af225384a24ac6301f987ce2e94f262500 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Wed, 8 Dec 2021 17:06:03 +0900
Subject: [PATCH] Cygwin: clipboard: Make intent of the code clearer.

---
 winsup/cygwin/fhandler_clipboard.cc   | 4 ++--
 winsup/cygwin/include/sys/clipboard.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler_clipboard.cc b/winsup/cygwin/fhandler_clipboard.cc
index 05f54ffb3..65a3cad97 100644
--- a/winsup/cygwin/fhandler_clipboard.cc
+++ b/winsup/cygwin/fhandler_clipboard.cc
@@ -76,7 +76,7 @@ fhandler_dev_clipboard::set_clipboard (const void *buf, size_t len)
       clipbuf->cb_sec  = clipbuf->ts.tv_sec;
 #endif
       clipbuf->cb_size = len;
-      memcpy (&clipbuf[1], buf, len); // append user-supplied data
+      memcpy (clipbuf->data, buf, len); // append user-supplied data
 
       GlobalUnlock (hmem);
       EmptyClipboard ();
@@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& len)
       if (pos < (off_t) clipbuf->cb_size)
 	{
 	  ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - pos : len;
-	  memcpy (ptr, (char *) (clipbuf + 1) + pos, ret);
+	  memcpy (ptr, clipbuf->data + pos, ret);
 	  pos += ret;
 	}
     }
diff --git a/winsup/cygwin/include/sys/clipboard.h b/winsup/cygwin/include/sys/clipboard.h
index 4c00c8ea1..b2544be85 100644
--- a/winsup/cygwin/include/sys/clipboard.h
+++ b/winsup/cygwin/include/sys/clipboard.h
@@ -44,6 +44,7 @@ typedef struct
     };
   };
   uint64_t      cb_size; // 8 bytes everywhere
+  char          data[];
 } cygcb_t;
 
 #endif
-- 
2.34.1


-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Mark Geisert]

Takashi Yano wrote:
[...]
> I think the following patch makes the intent clearer.
> What do you think?
> 
> 
>  From d0aee9af225384a24ac6301f987ce2e94f262500 Mon Sep 17 00:00:00 2001
> From: Takashi Yano <takashi.yano@nifty.ne.jp>
> Date: Wed, 8 Dec 2021 17:06:03 +0900
> Subject: [PATCH] Cygwin: clipboard: Make intent of the code clearer.
> 
> ---
>   winsup/cygwin/fhandler_clipboard.cc   | 4 ++--
>   winsup/cygwin/include/sys/clipboard.h | 1 +
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler_clipboard.cc b/winsup/cygwin/fhandler_clipboard.cc
> index 05f54ffb3..65a3cad97 100644
> --- a/winsup/cygwin/fhandler_clipboard.cc
> +++ b/winsup/cygwin/fhandler_clipboard.cc
> @@ -76,7 +76,7 @@ fhandler_dev_clipboard::set_clipboard (const void *buf, size_t len)
>         clipbuf->cb_sec  = clipbuf->ts.tv_sec;
>   #endif
>         clipbuf->cb_size = len;
> -      memcpy (&clipbuf[1], buf, len); // append user-supplied data
> +      memcpy (clipbuf->data, buf, len); // append user-supplied data
>   
>         GlobalUnlock (hmem);
>         EmptyClipboard ();
> @@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& len)
>         if (pos < (off_t) clipbuf->cb_size)
>   	{
>   	  ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - pos : len;
> -	  memcpy (ptr, (char *) (clipbuf + 1) + pos, ret);
> +	  memcpy (ptr, clipbuf->data + pos, ret);
>   	  pos += ret;
>   	}
>       }
> diff --git a/winsup/cygwin/include/sys/clipboard.h b/winsup/cygwin/include/sys/clipboard.h
> index 4c00c8ea1..b2544be85 100644
> --- a/winsup/cygwin/include/sys/clipboard.h
> +++ b/winsup/cygwin/include/sys/clipboard.h
> @@ -44,6 +44,7 @@ typedef struct
>       };
>     };
>     uint64_t      cb_size; // 8 bytes everywhere
> +  char          data[];
>   } cygcb_t;
>   
>   #endif

Sigh.  I guess it's not possible to keep rid of a data item like I'd hoped.  At 
least "data[]" is cleaner than the historical "data[1]" here.  If you call the 
item cb_data I can live with it.
Thanks all for the discussion.

..mark

Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Corinna Vinschen]

On Dec  8 01:43, Mark Geisert wrote:
> Takashi Yano wrote:
> [...]
> > I think the following patch makes the intent clearer.
> > What do you think?
> > 
> > 
> >  From d0aee9af225384a24ac6301f987ce2e94f262500 Mon Sep 17 00:00:00 2001
> > From: Takashi Yano <takashi.yano@nifty.ne.jp>
> > Date: Wed, 8 Dec 2021 17:06:03 +0900
> > Subject: [PATCH] Cygwin: clipboard: Make intent of the code clearer.
> > 
> > ---
> >   winsup/cygwin/fhandler_clipboard.cc   | 4 ++--
> >   winsup/cygwin/include/sys/clipboard.h | 1 +
> >   2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/winsup/cygwin/fhandler_clipboard.cc b/winsup/cygwin/fhandler_clipboard.cc
> > index 05f54ffb3..65a3cad97 100644
> > --- a/winsup/cygwin/fhandler_clipboard.cc
> > +++ b/winsup/cygwin/fhandler_clipboard.cc
> > @@ -76,7 +76,7 @@ fhandler_dev_clipboard::set_clipboard (const void *buf, size_t len)
> >         clipbuf->cb_sec  = clipbuf->ts.tv_sec;
> >   #endif
> >         clipbuf->cb_size = len;
> > -      memcpy (&clipbuf[1], buf, len); // append user-supplied data
> > +      memcpy (clipbuf->data, buf, len); // append user-supplied data
> >         GlobalUnlock (hmem);
> >         EmptyClipboard ();
> > @@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& len)
> >         if (pos < (off_t) clipbuf->cb_size)
> >   	{
> >   	  ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - pos : len;
> > -	  memcpy (ptr, (char *) (clipbuf + 1) + pos, ret);
> > +	  memcpy (ptr, clipbuf->data + pos, ret);
> >   	  pos += ret;
> >   	}
> >       }
> > diff --git a/winsup/cygwin/include/sys/clipboard.h b/winsup/cygwin/include/sys/clipboard.h
> > index 4c00c8ea1..b2544be85 100644
> > --- a/winsup/cygwin/include/sys/clipboard.h
> > +++ b/winsup/cygwin/include/sys/clipboard.h
> > @@ -44,6 +44,7 @@ typedef struct
> >       };
> >     };
> >     uint64_t      cb_size; // 8 bytes everywhere
> > +  char          data[];
> >   } cygcb_t;
> >   #endif
> 
> Sigh.  I guess it's not possible to keep rid of a data item like I'd hoped.
> At least "data[]" is cleaner than the historical "data[1]" here.  If you
> call the item cb_data I can live with it.
> Thanks all for the discussion.

  sometype *ptr;

  ptr = (sometype *) somebuffer;
  do_something (ptr + 1);

is a perfectly valid and perfectly readable thing, and used a lot if
"sometype" is either a header in a buffer followed by arbitrary data, or
if the buffer consists of multiple packed blocks of type "sometype".

Takashi's suggestion adds the information that "sometype" is a header
followed by arbitrary data, so that's a good thing..


Corinna

Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().

[Thomas Wolff]

Am 08.12.2021 um 11:19 schrieb Corinna Vinschen:
> On Dec  8 01:43, Mark Geisert wrote:
>> Takashi Yano wrote:
>> [...]
>>> I think the following patch makes the intent clearer.
>>> What do you think?
>>>
>>>
>>>   From d0aee9af225384a24ac6301f987ce2e94f262500 Mon Sep 17 00:00:00 2001
>>> From: Takashi Yano <takashi.yano@nifty.ne.jp>
>>> Date: Wed, 8 Dec 2021 17:06:03 +0900
>>> Subject: [PATCH] Cygwin: clipboard: Make intent of the code clearer.
>>>
>>> ---
>>>    winsup/cygwin/fhandler_clipboard.cc   | 4 ++--
>>>    winsup/cygwin/include/sys/clipboard.h | 1 +
>>>    2 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/winsup/cygwin/fhandler_clipboard.cc b/winsup/cygwin/fhandler_clipboard.cc
>>> index 05f54ffb3..65a3cad97 100644
>>> --- a/winsup/cygwin/fhandler_clipboard.cc
>>> +++ b/winsup/cygwin/fhandler_clipboard.cc
>>> @@ -76,7 +76,7 @@ fhandler_dev_clipboard::set_clipboard (const void *buf, size_t len)
>>>          clipbuf->cb_sec  = clipbuf->ts.tv_sec;
>>>    #endif
>>>          clipbuf->cb_size = len;
>>> -      memcpy (&clipbuf[1], buf, len); // append user-supplied data
>>> +      memcpy (clipbuf->data, buf, len); // append user-supplied data
>>>          GlobalUnlock (hmem);
>>>          EmptyClipboard ();
>>> @@ -229,7 +229,7 @@ fhandler_dev_clipboard::read (void *ptr, size_t& len)
>>>          if (pos < (off_t) clipbuf->cb_size)
>>>    	{
>>>    	  ret = (len > (clipbuf->cb_size - pos)) ? clipbuf->cb_size - pos : len;
>>> -	  memcpy (ptr, (char *) (clipbuf + 1) + pos, ret);
>>> +	  memcpy (ptr, clipbuf->data + pos, ret);
>>>    	  pos += ret;
>>>    	}
>>>        }
>>> diff --git a/winsup/cygwin/include/sys/clipboard.h b/winsup/cygwin/include/sys/clipboard.h
>>> index 4c00c8ea1..b2544be85 100644
>>> --- a/winsup/cygwin/include/sys/clipboard.h
>>> +++ b/winsup/cygwin/include/sys/clipboard.h
>>> @@ -44,6 +44,7 @@ typedef struct
>>>        };
>>>      };
>>>      uint64_t      cb_size; // 8 bytes everywhere
>>> +  char          data[];
>>>    } cygcb_t;
>>>    #endif
>> Sigh.  I guess it's not possible to keep rid of a data item like I'd hoped.
>> At least "data[]" is cleaner than the historical "data[1]" here.  If you
>> call the item cb_data I can live with it.
>> Thanks all for the discussion.
>    sometype *ptr;
>
>    ptr = (sometype *) somebuffer;
>    do_something (ptr + 1);
>
> is a perfectly valid and perfectly readable thing, and used a lot if
> "sometype" is either a header in a buffer followed by arbitrary data, or
> if the buffer consists of multiple packed blocks of type "sometype".
>
> Takashi's suggestion adds the information that "sometype" is a header
> followed by arbitrary data, so that's a good thing..
Yes, thanks for this variant.
Thomas