From: Brian Inglis <Brian.Inglis@SystematicSw.ab.ca>
Subject: Re: [PATCH v2] Cygwin: clipboard: Fix a bug in read().
Date: Wed, 8 Dec 2021 00:24:14 -0700 [thread overview]
Message-ID: <25fe900a-f2d1-1688-d7af-95af7fc599b2@SystematicSw.ab.ca> (raw)
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'.
>>>>> 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
>>>>> 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 + pos , ret);
>>>>> + memcpy (ptr, (char *) &clipbuf + 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) + pos, ret);
>>> or even
>>> + memcpy (ptr, ((char *) (&clipbuf)) + 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
> to mean "just after the defining struct" was OK. But the code needed a
> ptr to some char offset after that and
> &clipbuf + 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
> 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.]
next prev parent reply other threads:[~2021-12-08 7:24 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-07 14:00 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 [this message]
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
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).