Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

scrotGrabMousePointer: error out on failure #317

Merged
merged 2 commits into from
May 30, 2023
Merged

scrotGrabMousePointer: error out on failure #317

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14ed018
scrotGrabMousePointer: avoid malloc and fixup in place
N-R-K May 25, 2023
f93a551
scrotGrabMousePointer: error in case of failure
N-R-K May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 10 additions & 22 deletions src/scrot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,33 +448,24 @@ Window scrotGetWindow(Display *display, Window window, int x, int y)
void scrotGrabMousePointer(const Imlib_Image image, const int xOffset,
const int yOffset)
{
XFixesCursorImage *xcim = NULL;
uint32_t *pixels = NULL;
/* Get the X cursor and the information we want. */
if ((xcim = XFixesGetCursorImage(disp)) == NULL) {
N-R-K marked this conversation as resolved.
Show resolved Hide resolved
warnx("Can't get the cursor from X");
goto end;
}
XFixesCursorImage *xcim = XFixesGetCursorImage(disp);
if (!xcim)
errx(EXIT_FAILURE, "Can't get the cursor from X");
const unsigned short width = xcim->width;
const unsigned short height = xcim->height;
const size_t pixcnt = (size_t)width*height;

/* xcim->pixels wouldn't be valid if sizeof(*pixels)*pixcnt wrapped around.
*/
if ((pixels = malloc(sizeof(*pixels)*pixcnt)) == NULL) {
warn("malloc()");
goto end;
}
uint32_t *pixels = (uint32_t *)xcim->pixels;
/* XFixesCursorImage returns pixels as `unsigned long`, which is typically
* 64bits, but imlib2 expects 32bit packed integers. */
for (size_t i = 0; i < pixcnt; i++)
pixels[i] = xcim->pixels[i];
if (sizeof *xcim->pixels > sizeof *pixels) {
for (size_t i = 0; i < pixcnt; ++i)
pixels[i] = xcim->pixels[i];
}
Comment on lines +458 to +464
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does violate C's aliasing rules: https://www.iso-9899.info/n1570.html#6.5p7

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've read that and explained in the commit why there's no violation. If you think otherwise then feel free to explain in more detail.

Copy link
Collaborator Author

@N-R-K N-R-K May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elaborate, each element of an array is an object by itself (C99 § 3.14). The write into u32[i] only changes the effective type of that region, not the entire array, the rest of the elements still retain their effective type because they haven't been written to, C99 § 6.5:

If a value is stored into an object having no declared type through an lvalue having a type that is not a character type, then the type of the lvalue becomes the effective type of the object for that access and for subsequent accesses that do not modify the stored value.

Copy link
Contributor

@guijan guijan May 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's perfectly valid as far as C goes that xcim->pixels is aligned to unsigned long, but not uint32_t. There is no requirement that uint32_t's alignment requirements have to be <= unsigned long's, or that unsigned long's alignment requirements have to be an integer multiple of uint32_t's.

Perhaps we can let it slip, Eric S. Raymond's struct packing guide says:

I’ve learned something rather reassuring from working with the source code for the reference implementation of NTP. It does packet analysis by reading packets off the wire directly into memory that the rest of the code sees as a struct, relying on the assumption of minimal self-aligned padding - or zero padding in odd cases like 690x0.

The interesting news is that NTP has apparently being getting away with this for decades across a very wide span of hardware, operating systems, and compilers, including not just Unixes but under Windows variants as well. This suggests that platforms with padding rules other than self-alignment are either nonexistent or confined to such specialized niches that they’re never either NTP servers or clients.

i.e. in all modern machines _Alignof(type) == sizeof(type), although I know that long double can be an exception on i386 with its 80-bit FPU: it has sizeof(long double) == 10 and _Alignof(long double) == 16 in the SysV ABI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, no unnecessary casts as in line 458 please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, no unnecessary casts as in line 458 please.

The cast wasn't there on my first iteration, but it caused a compiler warning. So I added it.

There is no requirement that uint32_t's alignment requirements have to be <= unsigned long's, or that unsigned long's alignment requirements have to be an integer multiple of uint32_t's.

Okay, that's fair. I was only thinking of aliasing and didn't consider alignment. But if such a machine exists, I kinda doubt Imlib2 will work there, because IIRC I saw some of the Imlib2 loaders treat raw memory as struct as well - which would most likely be broken under such machines.

Copy link
Contributor

@guijan guijan May 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it caused a compiler warning

Ugh, compilers forcing us to write C++-like C.

Edit: actually, I guess it's not the compiler's fault if we write known UB and it warns, the unnecessary cast is the "I know what I'm doing" idiom of C.


Imlib_Image imcursor = imlib_create_image_using_data(width, height, pixels);
if (!imcursor) {
warnx("Can't create cursor image");
goto end;
}
if (!imcursor)
errx(EXIT_FAILURE, "Can't create cursor image");

/* Overlay the cursor into `image`. */
const int x = (xcim->x - xcim->xhot) - xOffset;
Expand All @@ -486,9 +477,6 @@ void scrotGrabMousePointer(const Imlib_Image image, const int xOffset,
height);
imlib_context_set_image(imcursor);
imlib_free_image();

end:
free(pixels);
XFree(xcim);
}

Expand Down