Overflow in shmlog_size

[JordanP-Dev]

I'm submitting a…

[x] Bug
[ ] Feature Request
[ ] Documentation Request
[ ] Other (Please describe in detail)

Current Behavior

shmlog_size in src/log.c can overflow if system memory size is large. I have 256GB of memory in my system. 1% of that is too large, which causes shmlog_size to overflow, which then causes an invalid argument error to the ftruncate call.

Expected Behavior

shmlog_size should be a larger type so it doesn't overflow.

Reproduction Instructions

Run i3-with-shmlog session on machine with over 200GB of RAM.

Environment

Output of i3 --moreversion 2>&-:

i3 version: 4.20.1

Config file

Not config related.

Logfile URL:

- Linux Distribution & Version: Gentoo
- Are you using a compositor (e.g., xcompmgr or compton): No

[i3bot]

I don’t see a link to logs.i3wm.org. Did you follow https://i3wm.org/docs/debugging.html? (In case you actually provided a link to a logfile, please ignore me.)

[psychon]

Reproduction Instructions
Run i3-with-shmlog session on machine with over 200GB of RAM.

Well, yeah... uhm... no. :-D

Do you think the following is okay for reproducing?

diff --git a/src/log.c b/src/log.c
index 183bb8e6..bac228c1 100644
--- a/src/log.c
+++ b/src/log.c
@@ -137,7 +137,12 @@ void open_logbuffer(void) {
      * For 512 MiB of RAM this will lead to a 5 MiB log buffer.
      * At the moment (2011-12-10), no testcase leads to an i3 log
      * of more than ~ 600 KiB. */
+    physical_mem_bytes = 256ll * 1024 * 1024 * 1024;
+    printf("shmlog_size = %d, phys mem = %lld\n", shmlog_size, physical_mem_bytes);
+    int foo = physical_mem_bytes * 0.01;
+    printf("phys mem * 0.01 = %d\n", foo);
     logbuffer_size = min(physical_mem_bytes * 0.01, shmlog_size);
+    printf("log buffer size %d\n", logbuffer_size);
 #if defined(__FreeBSD__)
     sasprintf(&shmlogname, "/tmp/i3-log-%d", getpid());
 #else

With this, I get:

shmlog_size = 26214400, phys mem = 274877906944
phys mem * 0.01 = -2147483648
log buffer size -2147483648
Could not ftruncate SHM segment for the i3 log: Das Argument ist ungültig

One possible fix that I can think of is:

diff --git a/src/log.c b/src/log.c
index 183bb8e6..623da59b 100644
--- a/src/log.c
+++ b/src/log.c
@@ -26,6 +26,7 @@
 #include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/time.h>
+#include <math.h>
 #include <unistd.h>
 
 #if defined(__APPLE__)
@@ -137,7 +138,9 @@ void open_logbuffer(void) {
      * For 512 MiB of RAM this will lead to a 5 MiB log buffer.
      * At the moment (2011-12-10), no testcase leads to an i3 log
      * of more than ~ 600 KiB. */
-    logbuffer_size = min(physical_mem_bytes * 0.01, shmlog_size);
+    physical_mem_bytes = 256ll * 1024 * 1024 * 1024;
+    logbuffer_size = min(fmin(physical_mem_bytes * 0.01, INT_MAX), shmlog_size);
+    printf("log buffer size %d\n", logbuffer_size);
 #if defined(__FreeBSD__)
     sasprintf(&shmlogname, "/tmp/i3-log-%d", getpid());
 #else

Output:

log buffer size 26214400

I do not intend to open a PR with the above. Anyone is free to do that instead.

[stapelberg]

Thanks for taking a look, @psychon.

I went a slightly different route (avoiding the min() call to make clearer individual comparisons) in #5113. Would you mind glancing over that PR to see if it looks reasonable to you?

Thanks in advance

[psychon]

Sorry for being a week late. The patch in 1d03385 is definitely an improvement, but... does it always work? If physical_mem_bytes is above INT_MAX * 100... the implicit cast to int will still cause an overflow.

So... does anyone have a machine with more than 400 GiB of RAM (2^32 * 100 bytes)?

[stapelberg]

Are you sure there’s still a problem? I just tried it with this test program, and all 3 physical_mem_bytes variants result in 25 MB logbuffer_size for me:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <stdbool.h>
#include <stdint.h>
#include <limits.h>

int main() {
  int shmlog_size = 25 * 1024 * 1024;
  long long physical_mem_bytes;
  int logbuffer_size;

  physical_mem_bytes = 256ULL * 1024 * 1024 * 1024; // 256 GB
  // physical_mem_bytes = 512ULL * 1024 * 1024 * 1024; // 512 GB
  // physical_mem_bytes = (long long)INT_MAX * 200; // exceeds INT_MAX * 100

  logbuffer_size = shmlog_size;
  if (physical_mem_bytes * 0.01 < (long long)shmlog_size) {
    logbuffer_size = physical_mem_bytes * 0.01;
  }

  printf("logbuffer_size = %d\n", logbuffer_size);
}

When using gcc -Wconversion, I see:

tmp.eFwrGbIFMd.c:22:26: warning: conversion from ‘long long int’ to ‘double’ may change value [-Wconversion]
   22 |   if (physical_mem_bytes * 0.01 < (long long)shmlog_size) {
      |                          ^

But converting from long long int to double only means a loss in precision, not a loss in signedness, so the logic is still fine, no?

[psychon]

Argh, sorry! My fault. I mis-read the if as > instead of <. Put differently: I thought this was doing max, not min.

With such a large value the if is not taken and thus logbuffer_size = (200 * (long long) INT_MAX) * 0.01; is not executed and everything is fine.

Thanks for checking my ramblings.