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.]
