Run annocheck for libruby.so #11123

voxik · 2024-07-08T14:34:35Z

When building with --shared option, most functionality is kept in libruby.so. Therefore also run annocheck for libruby.so. Testing ruby executable is almost pointless.

voxik · 2024-07-08T14:38:02Z

A few thing to consider:

Not sure if the target should not depend also on existence of $(LIBRUBY_SO). IOW if this version would not be better:

$ git diff
diff --git a/common.mk b/common.mk
index 05145313c6..cfc474ca19 100644
--- a/common.mk
+++ b/common.mk
@@ -1667,8 +1667,8 @@ no-test-bundler-parallel:
 # The annocheck supports ELF format binaries compiled for any OS and for any
 # architecture. It is designed to be independent of the host OS and the
 # architecture. The test-annocheck.sh requires docker or podman.
-test-annocheck: $(PROGRAM)
-       $(tooldir)/test-annocheck.sh $(PROGRAM)
+test-annocheck: $(PROGRAM) $(LIBRUBY_SO)
+       $(tooldir)/test-annocheck.sh $(PROGRAM) $(LIBRUBY_SO)
 
 GEM = up
 sync-default-gems:

Not sure if it would not be good idea to test all *.so files
It could even make sense to test the *.o files, because that would provide more details, should there be some issue.

voxik · 2024-07-09T08:38:31Z

Apparently COPY $FILES /work/ treats ruby libruby.so.3.4.0 env variable as a single file (discussed here. So this won't be as easy as it could.

When building with `--shared` option, most functionality is kept in `libruby.so`. Therefore also run annocheck for `libruby.so`. Testing `ruby` executable is almost pointless. Use `sed` to expand the `${FILES}` variable, because Docker can't do it: https://stackoverflow.com/questions/61599384/specify-multiple-files-in-arg-to-copy-in-dockerfile

It is not needed to create dir explicitly, when the destination end by slash

voxik · 2024-07-09T12:38:30Z

Apparently COPY $FILES /work/ treats ruby libruby.so.3.4.0 env variable as a single file (discussed here. So this won't be as easy as it could.

I decided to expand the ${FILES} variable by sed instead.

voxik · 2024-07-10T07:44:31Z

Not sure if the build should not be configured with --disable-yjit, because Rust hides possible fortification issues:

Hardened: libruby.so.3.4.0: skip: cf-protection test because mixed Rust and C code - control flow protection is needed but not yet supported by Rust

voxik · 2024-07-17T16:50:52Z

Not sure if the build should not be configured with --disable-yjit, because Rust hides possible fortification issues:
Hardened: libruby.so.3.4.0: skip: cf-protection test because mixed Rust and C code - control flow protection is needed but not yet supported by Rust 

And just FTR, this is the Rust tracker:

rust-lang/rust#93754

nobu · 2024-07-22T07:17:44Z

tool/test-annocheck.sh

@@ -27,7 +27,8 @@ else
  # volume in container in container on GitHub Actions
  # <.github/workflows/compilers.yml>.
  TAG="${TAG}-copy"
-  "${DOCKER}" build --rm -t "${TAG}" --build-arg=FILES="${*}" -f ${TOOL_DIR}/annocheck/Dockerfile-copy .
+  sed -r "s/\\$\{FILES\}/${*}/" ${TOOL_DIR}/annocheck/Dockerfile-copy | \


Isn't / as the separator unsafe, as it is also a file separator.

@nobu I would agree with your opinion. the / is unsafe when the ${*} includes the file separator /.

However, I don't think we need to use the sed command. Because this hack just ignores the functionality of the --build-arg=FILES="${*}" as an argument. And I have alternative idea to deal with this limitation in a way to understand the logic easily in my opinion. I will share it soon.

junaruga

For someone who is interested in the annocheck test on CI. The test is executed in the .github/workflows/annocheck.yml calling the make test-annocheck calling tool/test-annocheck.sh.

I tested and debugged this PR on my following branch in my forked repository. The latest commit on the branch is my change proposal for this PR.

https://github.com/junaruga/ruby/tree/wip/v_annocheck-libruby-so-debug

Below is the diff. I think this way is better in terms of understanding the logic easily without using the sed hack making the --build-arg=FILES="${*}" in the docker build command meaningless.

What do you think?

diff --git a/tool/annocheck/Dockerfile-copy b/tool/annocheck/Dockerfile-copy
index 132febc86c..0a79f3a50a 100644
--- a/tool/annocheck/Dockerfile-copy
+++ b/tool/annocheck/Dockerfile-copy
@@ -1,6 +1,6 @@
 FROM docker.io/fedora:latest
-ARG FILES
+ARG IN_DIR

 RUN dnf -y install annobin-annocheck
-COPY ${FILES} /work/
+COPY ${IN_DIR} /work
 WORKDIR /work
diff --git a/tool/test-annocheck.sh b/tool/test-annocheck.sh
index f6a6bd8223..cbe833d97c 100755
--- a/tool/test-annocheck.sh
+++ b/tool/test-annocheck.sh
@@ -15,6 +15,7 @@ set -x
 DOCKER="$(command -v docker || command -v podman)"
 TAG=ruby-fedora-annocheck
 TOOL_DIR=$(dirname "${0}")
+TMP_DIR="tmp/annocheck"
 DOCKER_RUN_VOLUME_OPTS=

 if [ -z "${CI-}" ]; then
@@ -27,8 +28,13 @@ else
   # volume in container in container on GitHub Actions
   # <.github/workflows/compilers.yml>.
   TAG="${TAG}-copy"
-  sed -r "s/\\$\{FILES\}/${*}/" ${TOOL_DIR}/annocheck/Dockerfile-copy | \
-    "${DOCKER}" build --rm -t "${TAG}" --build-arg=FILES="${*}" -f - .
+  rm -rf "${TMP_DIR}"
+  mkdir -p "${TMP_DIR}"
+  for file in ${@}; do
+    cp -p "${file}" "${TMP_DIR}"
+  done
+  "${DOCKER}" build --rm -t "${TAG}" --build-arg=IN_DIR="${TMP_DIR}" -f ${TOOL_DIR}/annocheck/Dockerfile-copy .
+  rm -rf "${TMP_DIR}"
 fi

 "${DOCKER}" run --rm -t ${DOCKER_RUN_VOLUME_OPTS} "${TAG}" annocheck --verbose ${TEST_ANNOCHECK_OPTS-} "${@}"

I confirmed the make test-annocheck passed on GitHub Actions on my forked repository.
https://github.com/junaruga/ruby/actions/runs/10073821672/job/27848650745#step:10:106

junaruga · 2024-07-23T13:21:16Z

common.mk

@@ -1668,7 +1668,7 @@ no-test-bundler-parallel:
 # architecture. It is designed to be independent of the host OS and the
 # architecture. The test-annocheck.sh requires docker or podman.
 test-annocheck: $(PROGRAM)


The $(LIBRUBY_SO) is missed at this line.

junaruga · 2024-07-23T13:24:21Z

tool/test-annocheck.sh

@@ -27,7 +27,8 @@ else
  # volume in container in container on GitHub Actions
  # <.github/workflows/compilers.yml>.
  TAG="${TAG}-copy"
-  "${DOCKER}" build --rm -t "${TAG}" --build-arg=FILES="${*}" -f ${TOOL_DIR}/annocheck/Dockerfile-copy .
+  sed -r "s/\\$\{FILES\}/${*}/" ${TOOL_DIR}/annocheck/Dockerfile-copy | \


@nobu I would agree with your opinion. the / is unsafe when the ${*} includes the file separator /.

However, I don't think we need to use the sed command. Because this hack just ignores the functionality of the --build-arg=FILES="${*}" as an argument. And I have alternative idea to deal with this limitation in a way to understand the logic easily in my opinion. I will share it soon.

junaruga · 2024-07-23T13:38:24Z

By the way, below is an article about ${@} vs ${*}.

https://www.baeldung.com/linux/dollar-star-at

In most cases, using $@ or “$@” instead of $* is safer and more flexible when working with command-line arguments because they preserve the original argument structure and make it easier to work with individual arguments in our bash script.

junaruga · 2024-08-07T12:25:44Z

I did cherry-pick this PR, adding my change, and opened another PR #11324. So, I would close this PR. Thanks for your PR!

voxik force-pushed the annocheck-libruby-so branch 2 times, most recently from 3b319bf to baf9214 Compare July 8, 2024 15:05

voxik changed the title ~~Run annocheck for libruby.so~~ [WIP] Run annocheck for libruby.so Jul 8, 2024

voxik force-pushed the annocheck-libruby-so branch 7 times, most recently from 46dc2bf to d706a1a Compare July 8, 2024 18:14

voxik force-pushed the annocheck-libruby-so branch 5 times, most recently from fa82c4f to 6daff56 Compare July 9, 2024 10:51

voxik added 2 commits July 9, 2024 14:36

Simplify annocheck Docker file

28280f7

It is not needed to create dir explicitly, when the destination end by slash

voxik force-pushed the annocheck-libruby-so branch from 373c911 to 28280f7 Compare July 9, 2024 12:37

voxik changed the title ~~[WIP] Run annocheck for libruby.so~~ Run annocheck for libruby.so Jul 9, 2024

nobu reviewed Jul 22, 2024

View reviewed changes

junaruga self-requested a review July 23, 2024 13:20

junaruga requested changes Jul 23, 2024

View reviewed changes

junaruga mentioned this pull request Aug 7, 2024

Run annocheck for libruby.so #11324

Merged

junaruga closed this Aug 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run annocheck for libruby.so #11123

Run annocheck for libruby.so #11123

voxik commented Jul 8, 2024

voxik commented Jul 8, 2024

voxik commented Jul 9, 2024 •

edited

Loading

voxik commented Jul 9, 2024

voxik commented Jul 10, 2024

voxik commented Jul 17, 2024

nobu Jul 22, 2024

junaruga Jul 23, 2024 •

edited

Loading

junaruga left a comment •

edited

Loading

junaruga Jul 23, 2024

junaruga Jul 23, 2024 •

edited

Loading

junaruga commented Jul 23, 2024

junaruga commented Aug 7, 2024

Run annocheck for libruby.so #11123

Run annocheck for libruby.so #11123

Conversation

voxik commented Jul 8, 2024

voxik commented Jul 8, 2024

voxik commented Jul 9, 2024 • edited Loading

voxik commented Jul 9, 2024

voxik commented Jul 10, 2024

voxik commented Jul 17, 2024

nobu Jul 22, 2024

Choose a reason for hiding this comment

junaruga Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

junaruga left a comment • edited Loading

Choose a reason for hiding this comment

junaruga Jul 23, 2024

Choose a reason for hiding this comment

junaruga Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

junaruga commented Jul 23, 2024

junaruga commented Aug 7, 2024

voxik commented Jul 9, 2024 •

edited

Loading

junaruga Jul 23, 2024 •

edited

Loading

junaruga left a comment •

edited

Loading

junaruga Jul 23, 2024 •

edited

Loading