Closed mds1 closed 5 years ago
I believe the sloc
version is meant to be installed based on the input solidity files. Therefore hardcoding a version into Dockerfile
will not fix the issue if a contract with a non compatible solidity pragma is given as input.
Right now the automatic install of sloc
is handled by scripts/install_sloc.py
which installs the latest version (currently 0.4.25
) which is too high for the example contract (pragma solidity 0.4.24;
). I would rather propose to change the install script to add:
if not os.path.exists(binary):
install_solc(f'v{solc_version}', platform='linux')
after the binary path is set. This should then install the correct sloc
version based on the contracts provided if no install already exists.
Of course, this does not solve the Killed
message.
Good point about scripts/install_solc.py
. I had overlooked that, and just assumed it was using the hardcoded solc
version installed in lines 34-36 of the original Dockerfile
, shown here:
https://github.com/eth-sri/securify/blob/13d4784901e5e2d179ef504a30b9af1abe355451/Dockerfile#L34-L36
Now, though, why would we need the above lines in Dockerfile
if solc
is later installed using scripts/install_solc.py
? I think those lines can be removed.
If I understood your proposal correctly, I believe you are suggesting to add those lines into pysolc.py
, right after the binary path is determined on line 116, i.e. like this:
def compile_solfiles(files, proj_dir, solc_version=None, output_values=OUTPUT_VALUES):
# original function code was here...
if solc_version is None:
solc_version = min(map(parse_version, files),
key=lambda x: _version_to_tuple(x))
binary = os.path.join(Path.home(), f'.py-solc/solc-v{solc_version}/bin/solc')
# proposing to add these two lines
if not os.path.exists(binary):
install_solc(f'v{solc_version}', platform='linux')
# ...continue original function code
This seems like a good solution, since it allows the image to always use the proper solc
version without rebuilding the image. It also means you can remove scripts/install_solc.py
and the corresponding line in Dockerfile
, since solc
is now installed on each run.
This combination of changes (removing those Dockerfile
lines and installing the proper solc
version with each run) seem to work well in my brief testing.
A downside here is that by removing the above Dockerfile
lines the image cannot find solc
simply by running solc --version
Yes, that is where I would place the binary existence check.
I agree that the image not finding sloc
simply by running sloc --version
is a disadvantage, but considering the sloc
executable (and version) provided by this is not the one used to compile the source code, I would actually say it is better to not have at all (it can create confusion). As to why this install is explicit in the Dockerfile
in the first place, I have no idea.
Regarding removing the scripts/install_sloc.py
altogether, I would argue that installing the latest version of sloc
during the building of the docker image is not really problematic and caches at least one version of sloc
in the image. This would allow faster analysis on most contracts with non-strict pragmas as the latest version of solc
can be used. If this is removed, the existence check of the binary
if not os.path.join(binary):
is useless as no version of sloc
will ever be cached between docker container runs. Therefore you would indeed need a fresh install of sloc
on each run, which I believe not to be ideal.
I don't know much about Dockerfile
s or docker in general, but maybe a smart solution would be to let the user decide on which additional versions of sloc
he wants to have preinstalled in the docker image, hence not requiring an explicit install each run. I don't know if this is possible by just passing arguments to the Dockerfile
? I think that would be a good (at least temporary) solution because the user should know which solidity versions he is most likely to use.
I'm no Docker expert either, but my understanding of the typical Docker use case makes caching a user-chosen solc
between runs not very feasible.
To elaborate, a typical use case would be:
# Step 1: Pull an already built Securify image from https://hub.docker.com/
# This image will either have a chosen version of `solc` pre-installed, or no
# version at all, depending on the decision made
docker pull eth-sri/securify
# Step 2: Run Securify on your contracts
docker run -v $(pwd)/folder_with_solidity_files:/project eth-sri/securify
Note on step 1: Currently there is no image for this project on Docker Hub, but it looks like it'll happen eventually in #3.
The command in step 2 simply runs docker_run_securify
within the container. This would fetch the appropriate version of solc
, if necessary, and then exits the container. A newly installed version of solc
does not persist between runs of this command, as containers do not have persistent storage by default. On a related note, with additional changes, you could allow the user to pass in a solc
version using an environment variable like this:
docker run -v $(pwd)/folder_with_solidity_files:/project -e SOLC_VERSION=0.4.23 eth-sri/securify
Then you could check for the existence of the SOLC_VERSION
environment variable, and if it exists, download that version. But once the container exits, re-running the above command generates a new container and solc
must again be installed, even if using the same non-default version as the last call.
Personally, I think it's ok even if no solc
versions are installed by default, since the install is pretty quick. Including the latest solc
version with the image certainly can't hurt though.
To ensure users know what solc
version is being used, you could either (1) force users to specify versions with the environment variable, and/or (2) include the solc
version used in the JSON output
Oh yes never mind I did not consider that the image would be pulled from Docker Hub instead of installed by the user directly. So I guess the pre-install on the image is not really necessary / very useful.
The problem was that we only installed the latest version of solc, so the version will change whenever there is a new release. We now install all versions, it means that the docker image is slower to build (which is why I wanted to avoid this), but it will do for now.
I have checked the following
This issue is about the Docker image, so the outputs shown below were run within a Securify container built by following the README.
souffle
binary is available, output when runningsouffle
:solc --version
solc --bin-runtime MyContract.sol
solc --bin-runtime project/example.sol
outputs:N/A since this is in a Docker container
Steps to reproduce
Upon running
docker run securify
I receive the following output:Proposed solution
Assuming that the path it is looking for is the desired path of solc, I updated the
# install solc
part ofDockerfile
to this:After building the image I can now successfully run the demo using
docker run securify
. However, usingdocker run -v $(pwd)/contracts:/project securify
to run Securify on my own files fails with the messageKilled
.Google tells me this problem is due to the max size of the Java heap memory, which is set to 16GB in
docker_run_securify
. My machine only has 8GB of memory, and Docker is set to use a max of 4GB, so indocker_run_securify
I changed:securify_cmd="java -Xmx16G -jar /securify_jar/securify.jar -co"
to
securify_cmd="java -Xmx4G -jar /securify_jar/securify.jar -co"
Everything now seems to work properly. I could be missing something, but it seems
Dockerfile
anddocker_run_securify
should be updated with the above changes. Perhaps the max heap memory size can be optionally passed in as an input when running the container. I can submit a pull request with these changes if you'd like.