fabric8io-images / java

Java Base Images
Apache License 2.0
176 stars 99 forks source link

max_mem_meminfo integer overflow #16

Closed matelang closed 6 years ago

matelang commented 6 years ago

Hi,

we are using fabric8 images to run our spring-boot based java applications.

Since we updated to the latest version (44ef6f145b51 from dockerhub) we had problems with the container detecting & defining the right amount of memory. The container on startup does not include the XMX option.

Steps to reproduce

1) Start container with docker run -m2G - to limit memory to 2G of RAM - on a machine which has at least 16G of RAM 2) Run run-java.sh with arbitrary JAR 3) Note that there is no XMX flag present in the startup options

Bug description

We think that the issue is caused by an integer overflow in method max_memory(), line:

    local max_mem_meminfo="$(cat /proc/meminfo | awk '/MemTotal/ {printf "%d\n", $2 * 1024}')"

If the system's available memory is 16G then 'MemTotal' is:

/deployments # cat /proc/meminfo | grep 'MemTotal'
MemTotal:       16295228 kB

If you multiply that by 1024, it's going to overflow to the negative range:

/deployments # cat /proc/meminfo | awk '/MemTotal/ {printf "%d\n", $2 * 1024'}
-2147483648

Then the condition expressed by:

if [ ${max_mem_cgroup:-0} != -1 ] && [ ${max_mem_cgroup:-0} -lt ${max_mem_meminfo:-0} ]

is not going to be true, so CONTAINER_MAX_MEMORY will not be set and exported, causing the exec line to contain no memory related options.

Also, thanks for @arnoldtempfli for investigating this.

System info:

[mate@devmate ~]$ uname -a
Linux devmate 4.14.6-1-ARCH #1 SMP PREEMPT Thu Dec 14 21:26:16 UTC 2017 x86_64 GNU/Linux

[mate@devmate ~]$ docker --version
Docker version 17.11.0-ce, build 1caf76ce6b
astefanutti commented 6 years ago

Not sure if I'm missing something, though running the following for 32GB:

$ docker run --rm -it fabric8/java-jboss-openjdk8-jdk echo 32590456 | awk '{printf "%d\n", $1 * 1024}'
33372626944

seems to yield the correct result.

jamesnetherton commented 6 years ago

@astefanutti I think it's the Alpine image which has the problem. java-jboss-openjdk8-jdk is based on CentOS.

matelang commented 6 years ago

@jamesnetherton that's correct. we use the alpine image.

astefanutti commented 6 years ago

@jamesnetherton thanks. Weird, I'm still getting the correct result:

$ docker run --rm -it fabric8/java-alpine-openjdk8-jdk echo 32590456 | awk '{printf "%d\n", $1 * 1024}'
33372626944
matelang commented 6 years ago

That's weird. If I run just the following command it seems to work okay.

[mate@devmate ~]$ docker run --rm -it fabric8/java-alpine-openjdk8-jdk cat /proc/meminfo | awk '/MemTotal/ {printf "%d\n", $2 * 1024'}
16686313472
jamesnetherton commented 6 years ago

That works for me also, but weirdly, if I run interactively in the container it does not:

$ docker run -ti --rm fabric8/java-alpine-openjdk8-jdk sh
/ # echo 32590456 | awk '{printf "%d\n", $1 * 1024'}
-2147483648
matelang commented 6 years ago

@jamesnetherton Confirmed here as well.

[mate@devmate ~]$ docker run --rm -it fabric8/java-alpine-openjdk8-jdk /bin/sh
/ # cat /proc/meminfo | awk '/MemTotal/ {printf "%d\n", $2 * 1024'}
-2147483648

vs

[mate@devmate ~]$ docker run --rm -it fabric8/java-alpine-openjdk8-jdk cat /proc/meminfo | awk '/MemTotal/ {printf "%d\n", $2 * 1024'}
16686313472
astefanutti commented 6 years ago

Funny enough, the following seems to work:

$ docker run --rm -it fabric8/java-alpine-openjdk8-jdk /bin/sh
/ # echo 32590456 | awk '{printf "%s\n", $1 * 1024}'
33372626944
rhuss commented 6 years ago

@matelang If you run

 docker run --rm -it fabric8/java-alpine-openjdk8-jdk cat /proc/meminfo | awk '/MemTotal/ {printf "%d\n", $2 * 1024'}

then you feed it into your local awk with the pipe, not to the awk running in the container. So its in fact an issue of the alpine (or better, busybox's awk)

@astefanutti You are using '%s' and not '%d' which make a huge different.

docker run --rm -it fabric8/java-alpine-openjdk8-jdk /bin/sh
/ # echo 32590456 | awk '{printf "%s\n", $1 * 1024}'
33372626944
/ # echo 32590456 | awk '{printf "%d\n", $1 * 1024}'
-2147483648
/ #
astefanutti commented 6 years ago

then you feed it into your local awk with the pipe, not to the awk running in the container. So its in fact an issue of the alpine (or better, busybox's awk)

That's what I suspected afterwards :)

@astefanutti You are using '%s' and not '%d' which make a huge different.

Yes. It seems like it's only the formatting that overflows while the computation is fine. Do you think that's an acceptable work-around for integer operations?

rhuss commented 6 years ago

I just have to verify, whether %s is save as it seems that awk's multiplication works with >32 bit, but not printf's %d or %u

rhuss commented 6 years ago

@astefanutti yes. if %s is safe I'm happy to change this. Let me do a quick research first ...

astefanutti commented 6 years ago

@rhuss sure, you're the script-fu master here 😉!

matelang commented 6 years ago

Or alternatively you could use the following and then you are not depending on awk handling integers

/ # echo $(( $(grep '^MemTotal' /proc/meminfo | awk '$1=$1' | sed 's/MemTotal:\ \([0-9][0-9]*\).*$/\1/') * 2 * 1024 ))
33372626944
rhuss commented 6 years ago

There are many ways to skin the cat:

maxmem_kb=$(cat /proc/meminfo | awk '/MemTotal/ { print $2 }')

# with bash builtin as in your example 
# (however the run scripts is supposed to work not only on bash, 
# so this solution is not possible for us)
maxmem=$(( $maxmem_kb * 1024 ))

# with expr:
maxmem=$(expr $maxmem_kb \* 1024)

# with good old dc:
maxmem=$(echo $maxmem_kb 1024 \* p | dc)

Actually I tend to the expr solution as this is the most straight forward one.

leiboo commented 6 years ago

It's haven't updated in the repository fabric8io/java , i wish the repository master fix the issue in time.

astefanutti commented 6 years ago

@leiboo it's been fixed in fabric8io-images/run-java-sh and new versions of the images will be released and pushed ASAP.

rhuss commented 6 years ago

@leiboo I hope I can do it until the end of the week. Please ping me again if this should be not the case.

matelang commented 6 years ago

Hi. Do we have a timeline when the fix is going to be available on docker hub? Thanks.

rhuss commented 6 years ago

Sorry, a release is already long outstanding. I hope I can get it in this week (although beeing on the road at the moment)