From 6772c8d5fe10ce555438494f20100a473d17e4ed Mon Sep 17 00:00:00 2001 From: NRK Date: Sat, 27 May 2023 11:34:40 +0600 Subject: [PATCH 1/4] introduce miliToNanoSec() --- src/scrot.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/scrot.c b/src/scrot.c index 7f305275..6ab02e47 100644 --- a/src/scrot.c +++ b/src/scrot.c @@ -74,6 +74,7 @@ static size_t scrotHaveFileExtension(const char *, char **); static Imlib_Image scrotGrabFocused(void); static void applyFilterIfRequired(void); static Imlib_Image scrotGrabAutoselect(void); +static long miliToNanoSec(int); static void scrotWaitUntil(const struct timespec *); static Imlib_Image scrotGrabShotMulti(void); static Imlib_Image scrotGrabShotMonitor(void); @@ -145,7 +146,7 @@ int main(int argc, char *argv[]) * screenshot-taking above or it will skew the timing. */ clock_gettime(CLOCK_REALTIME, &timeStamp); - if (timeStamp.tv_nsec >= 500*1000*1000) { + if (timeStamp.tv_nsec >= miliToNanoSec(500)) { /* Round the timestamp to the nearest second. */ timeStamp.tv_sec++; } @@ -345,6 +346,11 @@ void scrotDoDelay(void) } } +static long miliToNanoSec(int ms) +{ + return ms * 1000L * 1000L; +} + /* scrotWaitUntil: clock_nanosleep with a simpler interface and no EINTR nagging */ static void scrotWaitUntil(const struct timespec *time) @@ -364,7 +370,7 @@ static void scrotWaitUntil(const struct timespec *time) tmp.tv_nsec = time->tv_nsec - tmp.tv_nsec; if (tmp.tv_nsec < 0) { tmp.tv_sec--; - tmp.tv_nsec += 1000000000L; + tmp.tv_nsec += miliToNanoSec(1000); } } while (nanosleep(&tmp, NULL) == -1 && errno == EINTR); } From 646f7130920087bd37eea79ab14ef3e67883a6ed Mon Sep 17 00:00:00 2001 From: NRK Date: Sat, 27 May 2023 11:34:40 +0600 Subject: [PATCH 2/4] introduce clockNow() this will be useful for converting some of the nanosleep calls into scrotSleepFor(). --- src/scrot.c | 19 +++++++++++++++++-- src/scrot.h | 3 +++ src/scrot_selection.c | 2 +- src/util.h | 12 ------------ 4 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/scrot.c b/src/scrot.c index 6ab02e47..4e704316 100644 --- a/src/scrot.c +++ b/src/scrot.c @@ -109,7 +109,7 @@ int main(int argc, char *argv[]) struct tm *tm; /* Get the time ASAP to reduce the timing error in case --delay is used. */ - clock_gettime(CONTINUOUS_CLOCK, &opt.delayStart); + opt.delayStart = clockNow(); atexit(uninitXAndImlib); @@ -351,6 +351,21 @@ static long miliToNanoSec(int ms) return ms * 1000L * 1000L; } +/* clockNow() has the exact same semantics as CLOCK_MONOTONIC. Except that on + * Linux, CLOCK_MONOTONIC does not progress while the system is suspended, so + * the non-standard CLOCK_BOOTTIME is used instead to avoid this bug. + */ +struct timespec clockNow(void) +{ + struct timespec ret; +#if defined(__linux__) + clock_gettime(CLOCK_BOOTTIME, &ret); +#else + clock_gettime(CLOCK_MONOTONIC, &ret); +#endif + return ret; +} + /* scrotWaitUntil: clock_nanosleep with a simpler interface and no EINTR nagging */ static void scrotWaitUntil(const struct timespec *time) @@ -360,7 +375,7 @@ static void scrotWaitUntil(const struct timespec *time) */ struct timespec tmp; do { - clock_gettime(CONTINUOUS_CLOCK, &tmp); + tmp = clockNow(); /* XXX: Use timespecsub(). OS X doesn't have that BSD macro, and libbsd * doesn't support OS X save for an unmaintained fork. libobsd supports diff --git a/src/scrot.h b/src/scrot.h index c1d30bca..b32a9c62 100644 --- a/src/scrot.h +++ b/src/scrot.h @@ -28,6 +28,8 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. #pragma once +#include + #include #include @@ -39,5 +41,6 @@ extern Screen *scr; Window scrotGetWindow(Display *, Window, int, int); int scrotGetGeometry(Window, int *, int *, int *, int *); void scrotNiceClip(int *, int *, int *, int *); +struct timespec clockNow(void); void scrotDoDelay(void); void scrotGrabMousePointer(const Imlib_Image, const int, const int); diff --git a/src/scrot_selection.c b/src/scrot_selection.c index 72de3c69..298e10eb 100644 --- a/src/scrot_selection.c +++ b/src/scrot_selection.c @@ -433,7 +433,7 @@ Imlib_Image scrotSelectionSelectMode(void) return NULL; if (!opt.delaySelection) { - clock_gettime(CONTINUOUS_CLOCK, &opt.delayStart); + opt.delayStart = clockNow(); scrotDoDelay(); } diff --git a/src/util.h b/src/util.h index 6cc7c787..34b63009 100644 --- a/src/util.h +++ b/src/util.h @@ -26,21 +26,9 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. #pragma once -#include #include #include -/* On Linux, CLOCK_MONOTONIC does not progress while the system is suspended, - * and an alternative non-standard clock which does not suffer from this problem - * called CLOCK_BOOTTIME is available. Scrot's CONTINUOUS_CLOCK has the exact - * same semantics as CLOCK_MONOTONIC, only it avoids this bug. - */ -#if defined(__linux__) - #define CONTINUOUS_CLOCK CLOCK_BOOTTIME -#else - #define CONTINUOUS_CLOCK CLOCK_MONOTONIC -#endif - #ifdef DEBUG #define scrotAssert(X) do { \ if (!(X)) { \ From a57459cc69e1d051f91696213d22780147480b8a Mon Sep 17 00:00:00 2001 From: NRK Date: Sat, 27 May 2023 11:34:40 +0600 Subject: [PATCH 3/4] rework scrotWaitUntil() => scrotSleepFor() absolute time is really awkward to use. scrotSleepFor() instead takes a starting timestamp and a relative millisecond offset from that start and returns back the ending timestamp. this, combined with clockNow(), makes the API easy to use and easy to replace the existing nanosleep calls with. --- src/scrot.c | 39 +++++++++++++++++++++++---------------- src/scrot.h | 1 + src/scrot_selection.c | 4 ++-- src/selection_edge.c | 3 +-- 4 files changed, 27 insertions(+), 20 deletions(-) diff --git a/src/scrot.c b/src/scrot.c index 4e704316..a063bb96 100644 --- a/src/scrot.c +++ b/src/scrot.c @@ -75,7 +75,6 @@ static Imlib_Image scrotGrabFocused(void); static void applyFilterIfRequired(void); static Imlib_Image scrotGrabAutoselect(void); static long miliToNanoSec(int); -static void scrotWaitUntil(const struct timespec *); static Imlib_Image scrotGrabShotMulti(void); static Imlib_Image scrotGrabShotMonitor(void); static Imlib_Image scrotGrabStackWindows(void); @@ -336,13 +335,11 @@ void scrotDoDelay(void) dprintf(STDERR_FILENO, "Taking shot in "); for (int i = opt.delay; i > 0; i--) { dprintf(STDERR_FILENO, "%d.. ", i); - opt.delayStart.tv_sec++; - scrotWaitUntil(&opt.delayStart); + opt.delayStart = scrotSleepFor(opt.delayStart, 1000); } dprintf(STDERR_FILENO, "0.\n"); } else { - opt.delayStart.tv_sec += opt.delay; - scrotWaitUntil(&opt.delayStart); + scrotSleepFor(opt.delayStart, opt.delay * 1000); } } @@ -366,13 +363,22 @@ struct timespec clockNow(void) return ret; } -/* scrotWaitUntil: clock_nanosleep with a simpler interface and no EINTR nagging +/* OpenBSD and OS X lack clock_nanosleep(), so we call nanosleep() and use a + * trivial algorithm to correct for drift. The end timespec is returned for + * callers that want it. EINTR is also dealt with. */ -static void scrotWaitUntil(const struct timespec *time) +struct timespec scrotSleepFor(struct timespec start, int ms) { - /* OpenBSD and OS X lack clock_nanosleep(), so we use a trivial algorithm to - * correct for drift and call nanosleep() everywhere. - */ + scrotAssert(ms >= 0); + struct timespec end = { + .tv_sec = start.tv_sec + (ms / 1000), + .tv_nsec = start.tv_nsec + miliToNanoSec(ms % 1000), + }; + if (end.tv_nsec >= miliToNanoSec(1000)) { + ++end.tv_sec; + end.tv_nsec -= miliToNanoSec(1000); + } + struct timespec tmp; do { tmp = clockNow(); @@ -381,13 +387,15 @@ static void scrotWaitUntil(const struct timespec *time) * doesn't support OS X save for an unmaintained fork. libobsd supports * OS X but doesn't have the macro yet. */ - tmp.tv_sec = time->tv_sec - tmp.tv_sec; - tmp.tv_nsec = time->tv_nsec - tmp.tv_nsec; + tmp.tv_sec = end.tv_sec - tmp.tv_sec; + tmp.tv_nsec = end.tv_nsec - tmp.tv_nsec; if (tmp.tv_nsec < 0) { - tmp.tv_sec--; + --tmp.tv_sec; tmp.tv_nsec += miliToNanoSec(1000); } - } while (nanosleep(&tmp, NULL) == -1 && errno == EINTR); + } while (nanosleep(&tmp, NULL) < 0 && errno == EINTR); + + return end; } /* Clip rectangle nicely */ @@ -450,8 +458,7 @@ int scrotGetGeometry(Window target, int *rx, int *ry, int *rw, int *rh) /* HACK: there doesn't seem to be any way to figure out whether the * raise request was accepted or rejected. so just sleep a bit to * give the WM some time to update. */ - struct timespec t = { .tv_nsec = 160 * 1000L * 1000L }; - while (nanosleep(&t, &t) < 0 && errno == EINTR); + scrotSleepFor(clockNow(), 160); } } stat = XGetWindowAttributes(disp, target, &attr); diff --git a/src/scrot.h b/src/scrot.h index b32a9c62..5f877944 100644 --- a/src/scrot.h +++ b/src/scrot.h @@ -42,5 +42,6 @@ Window scrotGetWindow(Display *, Window, int, int); int scrotGetGeometry(Window, int *, int *, int *, int *); void scrotNiceClip(int *, int *, int *, int *); struct timespec clockNow(void); +struct timespec scrotSleepFor(struct timespec, int); void scrotDoDelay(void); void scrotGrabMousePointer(const Imlib_Image, const int, const int); diff --git a/src/scrot_selection.c b/src/scrot_selection.c index 298e10eb..29514c8a 100644 --- a/src/scrot_selection.c +++ b/src/scrot_selection.c @@ -233,9 +233,9 @@ bool scrotSelectionGetUserSel(struct SelectionRect *selectionRect) ret = XGrabKeyboard(disp, root, False, GrabModeAsync, GrabModeAsync, CurrentTime); if (ret == AlreadyGrabbed) { int attempts = 20; - struct timespec delay = { 0, 50 * 1000L * 1000L }; + struct timespec t = clockNow(); do { - nanosleep(&delay, NULL); + t = scrotSleepFor(t, 50); ret = XGrabKeyboard(disp, root, False, GrabModeAsync, GrabModeAsync, CurrentTime); } while (--attempts > 0 && ret == AlreadyGrabbed); } diff --git a/src/selection_edge.c b/src/selection_edge.c index 2a9cae4d..e73efc3d 100644 --- a/src/selection_edge.c +++ b/src/selection_edge.c @@ -143,8 +143,7 @@ void selectionEdgeDestroy(void) * might not have been updated. a compositor might also buffer frames * adding latency. so wait a bit for the screen to update and the * selection borders to go away. */ - struct timespec t = { .tv_nsec = 80 * 1000L * 1000L }; - while (nanosleep(&t, &t) < 0 && errno == EINTR); + scrotSleepFor(clockNow(), 80); XFree(pe->classHint); } From ce8e7f7cea0b4873755c99675fc2170cf7c9d31e Mon Sep 17 00:00:00 2001 From: NRK Date: Wed, 31 May 2023 03:55:08 +0600 Subject: [PATCH 4/4] options: reduce max delay to INT_MAX/1000 avoids potential multiplication overflow when the seconds are converted into milliseconds. --- src/options.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/options.c b/src/options.c index 86a6fd6e..7dafed2d 100644 --- a/src/options.c +++ b/src/options.c @@ -390,7 +390,8 @@ void optionsParse(int argc, char *argv[]) opt.delaySelection = *optarg == 'b'; if (opt.delaySelection) ++optarg; - opt.delay = optionsParseNum(optarg, 0, INT_MAX, &errmsg); + /* NOTE: div 1000 so that converting to milliseconds doesn't overflow */ + opt.delay = optionsParseNum(optarg, 0, INT_MAX/1000, &errmsg); if (errmsg) { errx(EXIT_FAILURE, "option --delay: '%s' is %s", optarg, errmsg);