* [PATCH v2] Cygwin: clipboard: Fix a bug in read(). @ 2021-12-07 14:00 Takashi Yano 2021-12-07 14:23 ` Corinna Vinschen 0 siblings, 1 reply; 10+ messages in thread From: Takashi Yano @ 2021-12-07 14:00 UTC (permalink / raw) To: cygwin-patches - 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read(). 2021-12-07 14:00 [PATCH v2] Cygwin: clipboard: Fix a bug in read() Takashi Yano @ 2021-12-07 14:23 ` Corinna Vinschen 2021-12-07 20:18 ` Thomas Wolff 0 siblings, 1 reply; 10+ messages in thread From: Corinna Vinschen @ 2021-12-07 14:23 UTC (permalink / raw) To: cygwin-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read(). 2021-12-07 14:23 ` Corinna Vinschen @ 2021-12-07 20:18 ` Thomas Wolff 2021-12-08 5:53 ` Brian Inglis 0 siblings, 1 reply; 10+ messages in thread From: Thomas Wolff @ 2021-12-07 20:18 UTC (permalink / raw) To: cygwin-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read(). 2021-12-07 20:18 ` Thomas Wolff @ 2021-12-08 5:53 ` Brian Inglis 2021-12-08 6:30 ` Mark Geisert 0 siblings, 1 reply; 10+ messages in thread From: Brian Inglis @ 2021-12-08 5:53 UTC (permalink / raw) To: cygwin-patches 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.] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read(). 2021-12-08 5:53 ` Brian Inglis @ 2021-12-08 6:30 ` Mark Geisert 2021-12-08 7:24 ` Brian Inglis 2021-12-08 8:19 ` Takashi Yano 0 siblings, 2 replies; 10+ messages in thread From: Mark Geisert @ 2021-12-08 6:30 UTC (permalink / raw) To: cygwin-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read(). 2021-12-08 6:30 ` Mark Geisert @ 2021-12-08 7:24 ` Brian Inglis 2021-12-08 8:19 ` Takashi Yano 1 sibling, 0 replies; 10+ messages in thread From: Brian Inglis @ 2021-12-08 7:24 UTC (permalink / raw) To: cygwin-patches 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.] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read(). 2021-12-08 6:30 ` Mark Geisert 2021-12-08 7:24 ` Brian Inglis @ 2021-12-08 8:19 ` Takashi Yano 2021-12-08 9:43 ` Mark Geisert 1 sibling, 1 reply; 10+ messages in thread From: Takashi Yano @ 2021-12-08 8:19 UTC (permalink / raw) To: cygwin-patches 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read(). 2021-12-08 8:19 ` Takashi Yano @ 2021-12-08 9:43 ` Mark Geisert 2021-12-08 10:19 ` Corinna Vinschen 0 siblings, 1 reply; 10+ messages in thread From: Mark Geisert @ 2021-12-08 9:43 UTC (permalink / raw) To: cygwin-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read(). 2021-12-08 9:43 ` Mark Geisert @ 2021-12-08 10:19 ` Corinna Vinschen 2021-12-08 18:46 ` Thomas Wolff 0 siblings, 1 reply; 10+ messages in thread From: Corinna Vinschen @ 2021-12-08 10:19 UTC (permalink / raw) To: cygwin-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read(). 2021-12-08 10:19 ` Corinna Vinschen @ 2021-12-08 18:46 ` Thomas Wolff 0 siblings, 0 replies; 10+ messages in thread From: Thomas Wolff @ 2021-12-08 18:46 UTC (permalink / raw) To: cygwin-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-12-08 18:46 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-07 14:00 [PATCH v2] Cygwin: clipboard: Fix a bug in read() Takashi Yano 2021-12-07 14:23 ` Corinna Vinschen 2021-12-07 20:18 ` Thomas Wolff 2021-12-08 5:53 ` Brian Inglis 2021-12-08 6:30 ` Mark Geisert 2021-12-08 7:24 ` Brian Inglis 2021-12-08 8:19 ` Takashi Yano 2021-12-08 9:43 ` Mark Geisert 2021-12-08 10:19 ` Corinna Vinschen 2021-12-08 18:46 ` Thomas Wolff
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).