Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Include embedded BoringSSL into libsoter.a #681

Merged
merged 5 commits into from
Jul 23, 2020
Merged

Include embedded BoringSSL into libsoter.a #681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d5c400
Include embedded BoringSSL into libsoter.a
ilammy Jul 13, 2020
467d674
Use Bash from PATH
ilammy Jul 21, 2020
63c78bc
Avoid copying library for extraction
ilammy Jul 21, 2020
c241a4c
Add "scripts" directory to libthemis-src
ilammy Jul 21, 2020
10a2e9f
Avoid duplicate linkage of embedded BoringSSL
ilammy Jul 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ Changes that are currently in development and have not been released yet.

_Code:_

- **Core**

- Include embedded BoringSSL into Soter for convenience ([#681](https://github.com/cossacklabs/themis/pull/681)).

- **Android**

- AndroidThemis is now available on JCenter ([#679](https://github.com/cossacklabs/themis/pull/679)).
Expand Down
68 changes: 68 additions & 0 deletions scripts/merge-static-libs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#!/usr/bin/env bash
#
# Merge multiple static libraries into one.
#
# scripts/merge-static-libs.sh libtarget.a [libdep1.a ...]
#
# Libraries libdep1.a are merged into libtarget.a. That is, all object files
# from those libraries are copied into the target library.
#
# Note that no deduplication is performed so use this script only once.

set -euo pipefail

AR=${AR:-ar}

help() {
cat $0 | awk 'NR == 3, /^$/ { print substr($0, 3) }'
}

while [[ $# -gt 0 ]]
do
case "$1" in
-h|--help) help; exit;;
--) shift; break;;
-*)
echo >&2 "Unknown option: $1"
echo >&2
help
exit 1
;;
*)
break
;;
esac
done

if [[ $# -lt 1 ]]
then
echo >&2 "Invalid command-line: missing target library name"
Comment on lines +37 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the script is useless without at least two arguments: a target and minimum one source library.

Suggested change
if [[ $# -lt 1 ]]
then
echo >&2 "Invalid command-line: missing target library name"
if [[ $# -lt 2 ]]
then
echo >&2 "Invalid command-line: target library and at least one source library required"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the script is useless without at least two arguments: a target and minimum one source library.

Not so useless, it is intentionally made a no op if there are no libraries to merge into the target library. This makes its behavior similar to sum function. It is useful because the case when there are no embedded libraries is no longer a special case.

The makefile filters out *.a files from dependencies of libsoter.a and passes them to this script. If there are no *.a files in dependencies (e.g., when system OpenSSL is used) then merge-static-libs.sh is invoked with only build/libsoter.a and does nothing.

If this case is treated as an error, the Makefile will need to have a conditonal to avoid using merge-static-libs.sh which is unnecessary complexity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation.

echo >&2
help
exit 1
fi

target="$1"
shift

tempdir="$(mktemp -d)"
trap 'rm -rf "$tempdir"' EXIT

while [[ $# -gt 0 ]]
do
if [[ ! -f "$1" ]]
then
echo >&2 "No such file: $1"
exit 1
fi
# Cast a possibly relative path to the absolute one.
src="$(cd "$(dirname "$1")" >/dev/null 2>&1 && pwd)/$(basename "$1")"
cd "$tempdir"
"$AR" x "$src"
cd "$OLDPWD"
# Actually merge the contents of the library into the target one.
find "$tempdir" -type f -name '*.o' -print0 | xargs -0 "$AR" rs "$target"
# Clean up and go on.
rm -rf "$tempdir"/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any possible corner cases when this line will remove the whole user dir ~/*?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't unless $tempdir is messed with from outside the script. If it's empty, something before this point will fail (e.g., extraction) and the script will exit before running rm -rf /*.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should add a log message here? Something like "Preparing to remove $tempdir", and then "$tempdir is successfully removed".

even if we are 99.9% sure that this line won't destroy the universe, in a very bad case, people will see the log message to understand what went wrong.

shift
done
11 changes: 8 additions & 3 deletions src/soter/soter.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,18 @@ SOTER_AUD = $(patsubst $(SRC_PATH)/%,$(AUD_PATH)/%, $(SOTER_AUD_SRC))
FMT_FIXUP += $(patsubst %,$(OBJ_PATH)/%.fmt_fixup, $(SOTER_FMT_SRC))
FMT_CHECK += $(patsubst %,$(OBJ_PATH)/%.fmt_check, $(SOTER_FMT_SRC))

SOTER_STATIC = $(BIN_PATH)/$(LIBSOTER_A) $(SOTER_ENGINE_DEPS)
SOTER_STATIC = $(BIN_PATH)/$(LIBSOTER_A)

$(SOTER_OBJ): CFLAGS += -DSOTER_EXPORT

$(BIN_PATH)/$(LIBSOTER_A): CMD = $(AR) rcs $@ $(filter %.o, $^)
# First build Soter library, then merge embedded crypto engine libs into it.
# On macOS this may cause warnings about files with no symbols in BoringSSL,
# suppress those warnings with some Bash wizardry.
$(BIN_PATH)/$(LIBSOTER_A): CMD = $(AR) rcs $@ $(filter %.o, $^) \
&& scripts/merge-static-libs.sh $@ $(filter %.a, $^) \
$(if $(IS_MACOS),> >(grep -v 'has no symbols$$'))

$(BIN_PATH)/$(LIBSOTER_A): $(SOTER_OBJ)
$(BIN_PATH)/$(LIBSOTER_A): $(SOTER_OBJ) $(SOTER_ENGINE_DEPS)
@mkdir -p $(@D)
@echo -n "link "
@$(BUILD_CMD)
Expand Down
1 change: 1 addition & 0 deletions src/wrappers/themis/rust/libthemis-src/themis/scripts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
../../../../../../scripts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😁