Skip to content

remove last #ifdef #193

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

Merged
merged 2 commits into from
Sep 3, 2022
Merged

remove last #ifdef #193

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
936ea99
remove last #ifdef
guijan Jul 25, 2022
ca800a5
Declaration in the scope of the loop
daltomi Sep 3, 2022
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
19 changes: 5 additions & 14 deletions src/scrot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,21 +411,12 @@ void scrotGrabMousePointer(const Imlib_Image image, const int xOffset,
const unsigned short height = xcim->height;
const int x = (xcim->x - xcim->xhot) - xOffset;
const int y = (xcim->y - xcim->yhot) - yOffset;
DATA32 *pixels = NULL;
DATA32 data[width * height];
Copy link
Collaborator

@N-R-K N-R-K Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd avoid using VLA, especially unchecked like this.

Compiling with -Wvla shows this to be the only VLA in the codebase and it could be easily replaced by malloc/free.

Copy link
Collaborator

@N-R-K N-R-K Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually since unsigned long cannot be less than 32bits, you don't need malloc either. You can easily do the mutation in place. Though that would violate the strict aliasing rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would consider violating C's aliasing rules by reusing the buffer xcim->pixels points to a bug.

The original version of this PR used reallocarray() to get rid of the VLA, but it was rejected partly because using reallocarray() introduced a dependency on libobsd on OS X, so I kept the VLA in this version.
Looking at this code again, it would be a bug in X if it gave us a cursor that was too large to measure, and it wouldn't be able to allocate xcim->pixels anyway, so we can just call malloc(sizeof(DATA32)*width*height) and assume it doesn't wrap around.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just looked at the Imlib2 API and found a function that can help us. Can anyone of you test if this is correct?
Sorry for the rush, see you next week.

diff --git a/src/scrot.c b/src/scrot.c
index 8783602..274e3c4 100644
--- a/src/scrot.c
+++ b/src/scrot.c
@@ -411,12 +411,8 @@ void scrotGrabMousePointer(const Imlib_Image image, const int xOffset,
     const unsigned short height = xcim->height;
     const int x = (xcim->x - xcim->xhot) - xOffset;
     const int y = (xcim->y - xcim->yhot) - yOffset;
-    DATA32 data[width * height];
-    
-    for (size_t i = 0; i < width*height; i++)
-            data[i] = xcim->pixels[i];
 
-    Imlib_Image imcursor = imlib_create_image_using_data(width, height, data);
+    Imlib_Image imcursor = imlib_create_image_using_copied_data(width, height, (DATA32*)xcim->pixels);
 
     XFree(xcim);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just looked at the Imlib2 API and found a function that can help us. Can anyone of you test if this is correct?

I don't see how that helps. The problem here wasn't ownership, the problem was imlib2 expects 32bit pixels, but XFixesGetCursorImage gives us pixels in unsigned long which might not be 32bits (and is typically 64bits on modern platform).

I would consider violating C's aliasing rules by reusing the buffer xcim->pixels points to a bug.

-fno-strict-alising flag can be enforced in the build if you're worried. The following patch works fine with or without that flag.

diff --git a/src/scrot.c b/src/scrot.c
index 2567b31..d66244a 100644
--- a/src/scrot.c
+++ b/src/scrot.c
@@ -412,14 +412,16 @@ void scrotGrabMousePointer(const Imlib_Image image, const int xOffset,
     const unsigned short height = xcim->height;
     const int x = (xcim->x - xcim->xhot) - xOffset;
     const int y = (xcim->y - xcim->yhot) - yOffset;
-    DATA32 data[width * height];
-    
-    for (size_t i = 0; i < width*height; i++)
-            data[i] = xcim->pixels[i];
+    DATA32 *data = (DATA32 *)xcim->pixels;
 
-    Imlib_Image imcursor = imlib_create_image_using_data(width, height, data);
+    assert(sizeof(*xcim->pixels) >= sizeof(*data));
+    if (sizeof(*xcim->pixels) != sizeof(*data)) {
+        /* NB: this violates strict aliasing rule, but whatever. */
+        for (size_t i = 0; i < (size_t)width*height; i++)
+            data[i] = (DATA32)xcim->pixels[i];
+    }
 
-    XFree(xcim);
+    Imlib_Image imcursor = imlib_create_image_using_data(width, height, data);
 
     if (!imcursor) {
         errx(EXIT_FAILURE,
@@ -433,6 +435,7 @@ void scrotGrabMousePointer(const Imlib_Image image, const int xOffset,
         height);
     imlib_context_set_image(imcursor);
     imlib_free_image();
+    XFree(xcim);
 }
 
 // It assumes that the local variable 'scrot.c:Imlib_Image image' is in context


for (size_t i = 0; i < width*height; i++)
data[i] = xcim->pixels[i];

#ifdef __i386__
pixels = (DATA32 *)xcim->pixels;
#else
DATA32 data[width * height * 4];

unsigned int i;
for (i = 0; i < (width * height); i++)
data[i] = (DATA32)xcim->pixels[i];

pixels = data;
#endif

Imlib_Image imcursor = imlib_create_image_using_data(width, height, pixels);
Imlib_Image imcursor = imlib_create_image_using_data(width, height, data);

XFree(xcim);

Expand Down