Osfuzz docker sandbox #14760
Osfuzz docker sandbox #14760
Conversation
nikola-matic
force-pushed
the
osfuzz-docker-sandbox
branch
from
3e60389 to
d7565ac
Compare
|
|
There was a problem hiding this comment.
Seems like to solve the problem. Perhaps it would be better to rely on source provided by Ubuntu rather than on an unofficial fork of GMP though.
Other than that I just have some small corrections and general remarks about the image. The latter we could perhaps discuss with @bshastry and do later not to hold back this PR.
In any case, since @nikola-matic is off I'm going to take this over and make the corrections myself.
| @@ -91,35 +92,38 @@ RUN set -ex; \ | |||
| rm -rf /usr/src/libprotobuf-mutator | |||
|
|
|||
| # EVMONE | |||
| WORKDIR cd /usr/src | |||
There was a problem hiding this comment.
WORKDIR takes a path, not a command. Not sure why this even works :)
| WORKDIR cd /usr/src | |
| WORKDIR /usr/src |
There was a problem hiding this comment.
Definitely a typo, also not sure why this worked.
There was a problem hiding this comment.
After playing with it yesterday I noticed that this basically resulted in a dir called 'cd ' being created at FS root :)
There was a problem hiding this comment.
That's definitely what happens, since WOKRDIR is basically just a mkdir and cd - what I was confused about was what happens to the 'second' directory in this case, i.e. /usr/src. Couldn't find anything in the official reference, so I would assume that it just gets ignored.
| RUN rm -rf /usr/src/gmp-6.2.1; \ | ||
| rm -f /usr/src/gmp-6-2.tar.gz |
There was a problem hiding this comment.
Putting this in a separate command makes it ineffective. Each docker command starts a new layer and removing files just marks them as not present in that layer. They still remain in the image though.
There was a problem hiding this comment.
Actually looks like at the end of the image build we're only taking content of /usr/{lib,bin,include}/ so I cleaning up does not matter that much. It won't remain in the final image.
But then we don't need to remove these files at all.
| tar -xf gmp.tar.xz; \ | ||
| cd gmp-6.2.1; \ | ||
| ./configure --prefix=/usr --enable-static=yes; \ | ||
| git clone --depth 1 --branch "gmp-6.2.1" https://github.com/gmp-mirror/gmp-6.2.git; \ |
There was a problem hiding this comment.
This is an unofficial mirror and I'm not sure how trustworthy it is and whether it will keep getting updated. Instead of trying to figure it out, perhaps it would be a better idea to just build from Ubuntu's source package instead. I'd generally recommend using distro packages for binaries anyway (when possible) and this would be equivalent.
echo "deb-src http://archive.ubuntu.com/ubuntu/ focal main restricted" >> /etc/apt/sources.list
apt update
apt install dpkg-dev
apt source gmp
This will leave sources in gmp-6.2.0+dfsg in the current dir.
Also, to prevent extra cached junk from remaining in the image:
apt remove --assume-yes dpkg-dev
apt autoremove --assume-yes
rm -r /var/lib/apt/lists/
rm -r gmp-*
What do you think @bshastry?
There was a problem hiding this comment.
Nice. For now in #14767 I kept the mirror used here but I'll create a separate PR with refactors where I'll include this and other changes.
One nice thing is that this will let us drop the hard-coded hash, which is a bit problematic when the source comes from a git repo. @nikola-matic hashed the whole git checkout, which seems somewhat reproducible in CI but gave me a hard time when I was trying to update it and get the right hash on my machine. There are multiple things making it non-reproduible - the inclusion of .git dir, tar including dates and permissions and finally even the fact that CI has a different timezone and trying to set the time with --mtime=1970-01-01 wasn't giving me the same timestamp. Being able to just rely on the fact that Ubuntu's source repos are trustworthy will save us this kind of headache in the future.
| # Install cmake 3.21.2 (minimum requirement is cmake 3.10) | ||
| RUN wget https://github.com/Kitware/CMake/releases/download/v3.21.2/cmake-3.21.2-Linux-x86_64.sh; \ | ||
| RUN wget -q https://github.com/Kitware/CMake/releases/download/v3.21.2/cmake-3.21.2-Linux-x86_64.sh; \ | ||
| test "$(sha256sum cmake-3.21.2-Linux-x86_64.sh)" = "3310362c6fe4d4b2dc00823835f3d4a7171bbd73deb7d059738494761f1c908c cmake-3.21.2-Linux-x86_64.sh"; \ | ||
| chmod +x cmake-3.21.2-Linux-x86_64.sh; \ | ||
| ./cmake-3.21.2-Linux-x86_64.sh --skip-license --prefix="/usr" |
There was a problem hiding this comment.
@bshastry Do we even need this? I see that focal ships cmake 3.16, which, according to the comment above, should work too. So perhaps we could remove this and simplify the image.
| make install | ||
| RUN rm -rf /usr/src/gmp-6.2.1; \ | ||
| rm -f /usr/src/gmp-6-2.tar.gz | ||
| WORKDIR /solidity | ||
|
|
||
| # libabicoder | ||
| RUN set -ex; \ |
There was a problem hiding this comment.
@bshastry I think this needs an update. Looks like The upstream repo has ~60 new commits (only mmalvarez/Yul-Isabelle@9adc718 seems missing) over @ekpyron's fork, though it also stopped being updated 3 years ago. Do we actually still need a personal fork here?
There was a problem hiding this comment.
This is fine as is - I'd not call the repo you're referring to "upstream" - that'd be https://github.com/ethereum/Yul-Isabelle these days really, if anything (but there we may have actually dropped the abi encoding stuff that we need here) - but we don't need to touch this at all - this was academic work, so there was never a clear repo separation between the formal abi encoder and proper yul semantics (that's now also abandoned), but the abi encoder formalization that's relevant here hasn't been updated from the state used here. Also not sure if anything but my fork has a convenient setup for the code generation part. (EDIT: ah yeah, that's that commit you referr to - but anyways)
Long story short: don't touch this unless it breaks.
There was a problem hiding this comment.
Hmmm... well, ok. The thing that bothers me the most here is the use of a personal repo, as I know from experience that these tend to disappear as people owning them leave the project and lose interest in not only maintaining, but even just preserving them. I don't think this is a big risk in your case, but I'd prefer to avoid those as a general rule. I was actually going to suggest to maybe transfer the repo to the ethereum org, but looks like there actually is one. What do you think about pushing your changes there and switching to it just for the peace of mind? Would that be a lot of effort?
There was a problem hiding this comment.
It doesn't really belong in that repo in the first place, the abi encoder formalization is independent of the yul formalization.
But yeah, in this case here: this was a time-boxed Grants project between Mario at Consensus and myself - his personal repo is not really more upstream, we just reused the old repo before we moved the Yul formalization part to the ethereum-repo.
None of this is critical in any case - if this vanishes for whatever reason, we can just drop this here anyways, so as I said: I'd just not touch this until it breaks and if it does probably just drop it.
|
Closing this since the relevant parts were merged in #14767. |
No description provided.