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.

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

[CI][Archery] Archery linking should also check for undefined symbols #40018

Open
3 tasks
raulcd opened this issue Feb 9, 2024 · 14 comments
Open
3 tasks

[CI][Archery] Archery linking should also check for undefined symbols #40018

raulcd opened this issue Feb 9, 2024 · 14 comments

Comments

@raulcd
Copy link
Member

raulcd commented Feb 9, 2024

In order to avoid issues like the one referenced below:

def check_dynamic_library_dependencies(path, allowed, disallowed):
dylib = DynamicLibrary(path)
for dep in dylib.list_dependency_names():
if allowed and dep not in allowed:
raise DependencyError(
f"Unexpected shared dependency found in {dylib.path}: `{dep}`"
)
if disallowed and dep in disallowed:
raise DependencyError(
f"Disallowed shared dependency found in {dylib.path}: `{dep}`"
)
should also check undefined symbols.

It seems that #39137 is the cause of this. Sorry...

Originally posted by @kou in #39919 (comment)

List of related issues:

@raulcd raulcd changed the title https://github.com/apache/arrow/blob/a0dec7f39394e619c8bdfe0eacb6ecde73a9ec12/dev/archery/archery/linking.py#L65-L75 should also check undefined symbols. [CI][Archery] Archery linking should also check for undefined symbols Feb 9, 2024
@danepitkin danepitkin added this to the 16.0.0 milestone Mar 8, 2024
@danepitkin
Copy link
Member

Hey @kou , by chance is this something you could take on?

@kou
Copy link
Member

kou commented Mar 9, 2024

I didn't have a plan to do this but I may work on this later.

BTW, I think that this is not difficult (at least for .so). We just need check symbols detected by nm XXX.so | grep '^ *U '.

@danepitkin danepitkin modified the milestones: 16.0.0, 15.0.2 Mar 12, 2024
@danepitkin
Copy link
Member

@raulcd This should probably be something we add to 15.0.2 as a failsafe.

@vibhatha
Copy link
Collaborator

@kou something like this could work right?

# Check for undefined symbols
result = subprocess.run(['nm', '-u', path], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
undefined_symbols = result.stdout.decode('utf-8')
if undefined_symbols:
    raise DependencyError(
        f"Undefined symbols found in {dylib.path}: {undefined_symbols}"
    )

How can we test this though? cc @raulcd

@kou
Copy link
Member

kou commented Mar 13, 2024

Wow! I didn't know the nm's -u option!
But your code will not work because there are some expected undefined symbols. They are symbols provided by linked libraries. See --allow options for linked libraries:

--allow ld-linux-aarch64 \
--allow ld-linux-x86-64 \
--allow libc \
--allow libdl \
--allow libgcc_s \
--allow libm \
--allow libpthread \
--allow librt \
--allow libstdc++ \
--allow libz \
--allow linux-vdso \

We need to remove expected undefined symbols from undefined_symbols. If there are still one or more undefined symbols, we should report an error.

@kou
Copy link
Member

kou commented Mar 13, 2024

We can test this feature by downloading arrow-dataset-X.Y.Z.jar (see #39919 (comment) for the URLs of them) and run the following command line:

archery linking check-dependencies \
  --allow ld-linux-aarch64 \
  --allow ld-linux-x86-64 \
  --allow libc \
  --allow libdl \
  --allow libgcc_s \
  --allow libm \
  --allow libpthread \
  --allow librt \
  --allow libstdc++ \
  --allow libz \
  --allow linux-vdso \
  arrow-dataset-X.Y.Z/x86_64/libarrow_dataset_jni.so

@vibhatha
Copy link
Collaborator

@kou thanks for this wonderful guideline. I will work on this.

@vibhatha
Copy link
Collaborator

@kou I have a draft PR: #40520
But I am not very confident with the matching criterion. Locally I tested, it clears undefined symbols from 777 to 478 from the allowed symbols. I used this: https://repo1.maven.org/maven2/org/apache/arrow/arrow-dataset/15.0.0/arrow-dataset-15.0.0.jar

I think this needs more work and proper validation. Appreciate your input.

cc @raulcd @danepitkin

@vibhatha
Copy link
Collaborator

I can locally get the following output

Error: Undefined symbols found in /home/asus/sandbox/ci/arrow-dataset-15.0.0/x86_64/libarrow_dataset_jni.so:
EVP_MD_CTX_create
EVP_MD_CTX_destroy
HMAC_CTX_cleanup
HMAC_CTX_init
_ZN6google8protobuf11MessageLite15ParseFromStringERKSs
_ZN6google8protobuf16RepeatedPtrFieldISsE3AddEv
_ZN6google8protobuf16RepeatedPtrFieldISsE5ClearEv
_ZN6google8protobuf16RepeatedPtrFieldISsE7ReserveEi
_ZN6google8protobuf16RepeatedPtrFieldISsE9MergeFromERKS2_
_ZN6google8protobuf16RepeatedPtrFieldISsEC1EPNS0_5ArenaE
_ZN6google8protobuf16RepeatedPtrFieldISsEC1ERKS2_
_ZN6google8protobuf16RepeatedPtrFieldISsED1Ev
_ZN6google8protobuf2io18StringOutputStreamC1EPSs
_ZN6google8protobuf2io19EpsCopyOutputStream18WriteStringOutlineEjRKSsPh
_ZN6google8protobuf2io19EpsCopyOutputStream30WriteStringMaybeAliasedOutlineEjRKSsPh
_ZN6google8protobuf3Any9MergeImplERNS0_7MessageERKS2_
_ZN6google8protobuf4util15status_internallsERSoRKNS2_6StatusE
_ZN6google8protobuf4util18BinaryToJsonStreamEPNS1_12TypeResolverERKSsPNS0_2io19ZeroCopyInputStreamEPNS6_20ZeroCopyOutputStreamERKNS1_16JsonPrintOptionsE
_ZN6google8protobuf4util18JsonToBinaryStreamEPNS1_12TypeResolverERKSsPNS0_2io19ZeroCopyInputStreamEPNS6_20ZeroCopyOutputStreamERKNS1_16JsonParseOptionsE
_ZN6google8protobuf4util32NewTypeResolverForDescriptorPoolERKSsPKNS0_14DescriptorPoolE
_ZN6google8protobuf5Arena23AllocateAlignedWithHookEmPKSt9type_info
_ZN6google8protobuf5Arena26AllocateAlignedWithCleanupEmPKSt9type_info
_ZN6google8protobuf7Message19CopyWithSourceCheckERS1_RKS1_
_ZN6google8protobuf8internal10VerifyUTF8ENS0_20stringpiece_internal11StringPieceEPKc
_ZN6google8protobuf8internal14ArenaStringPtr12ClearToEmptyEv
_ZN6google8protobuf8internal14ArenaStringPtr3SetEOSsPNS0_5ArenaE
_ZN6google8protobuf8internal14ArenaStringPtr3SetERKSsPNS0_5ArenaE
_ZN6google8protobuf8internal14ArenaStringPtr7DestroyEv
_ZN6google8protobuf8internal14ArenaStringPtr7MutableEPNS0_5ArenaE
_ZN6google8protobuf8internal14WireFormatLite20InternalWriteMessageEiRKNS0_11MessageLiteEiPhPNS0_2io19EpsCopyOutputStreamE
_ZN6google8protobuf8internal14ZeroFieldsBase14_InternalParseEPKcPNS1_12ParseContextE
_ZN6google8protobuf8internal14ZeroFieldsBase5ClearEv
_ZN6google8protobuf8internal14ZeroFieldsBase8CopyImplERNS0_7MessageERKS3_
_ZN6google8protobuf8internal14ZeroFieldsBase9MergeImplERNS0_7MessageERKS3_
_ZN6google8protobuf8internal14ZeroFieldsBaseD2Ev
_ZN6google8protobuf8internal15ThreadSafeArena10AddCleanupEPvPFvS3_E
_ZN6google8protobuf8internal15ThreadSafeArenaD1Ev
_ZN6google8protobuf8internal17AssignDescriptorsEPFPKNS1_15DescriptorTableEvEPSt9once_flagRKNS0_8MetadataE
_ZN6google8protobuf8internal18EpsCopyInputStream12DoneFallbackEii
_ZN6google8protobuf8internal20AddDescriptorsRunnerC1EPKNS1_15DescriptorTableE
_ZN6google8protobuf8internal20RepeatedPtrFieldBase13DestroyProtosEv
_ZN6google8protobuf8internal20RepeatedPtrFieldBase18AddOutOfLineHelperEPv
_ZN6google8protobuf8internal24InlineGreedyStringParserEPSsPKcPNS1_12ParseContextE
_ZN6google8protobuf8internal26fixed_address_empty_stringE
_ZNK6google8protobuf11MessageLite17SerializeAsStringEv
_ZNK6google8protobuf11MessageLite17SerializeToStringEPSs
_ZNK6google8protobuf16RepeatedPtrFieldISsE3endEv
_ZNK6google8protobuf16RepeatedPtrFieldISsE3GetEi
_ZNK6google8protobuf16RepeatedPtrFieldISsE4sizeEv
_ZNK6google8protobuf16RepeatedPtrFieldISsE5beginEv
_ZNK6google8protobuf16RepeatedPtrFieldISsE5emptyEv
_ZNK6google8protobuf7Message11DebugStringEv
_ZNK6google8protobuf7Message11GetTypeNameEv
_ZNK6google8protobuf7Message25InitializationErrorStringEv
_ZNK6google8protobuf7Message29MaybeComputeUnknownFieldsSizeEmPNS0_8internal10CachedSizeE
_ZNK6google8protobuf8internal11AnyMetadata10InternalIsENS0_20stringpiece_internal11StringPieceE
_ZNK6google8protobuf8internal14ZeroFieldsBase12ByteSizeLongEv
_ZNK6google8protobuf8internal14ZeroFieldsBase18_InternalSerializeEPhPNS0_2io19EpsCopyOutputStreamE
_ZTIN6google8protobuf8internal14ZeroFieldsBaseE

But my nm version could be different compared to what is in the CIs

@raulcd raulcd modified the milestones: 15.0.2, 16.0.0 Mar 14, 2024
@vibhatha
Copy link
Collaborator

vibhatha commented Apr 2, 2024

@kou I got a bit stuck in this PR with macOS changes. Is there an equivalent operation like ldconfig for MacOS? I am not very familiar, though finding paths in lib paths is one option.

@kou
Copy link
Member

kou commented Apr 2, 2024

How about splitting this issue to the following sub issues?

@vibhatha
Copy link
Collaborator

vibhatha commented Apr 2, 2024

Will do that @kou
Thank you for the suggestion.

@vibhatha
Copy link
Collaborator

vibhatha commented Apr 3, 2024

The above referenced issues have been created to track this issue.

@vibhatha
Copy link
Collaborator

vibhatha commented Apr 3, 2024

@raulcd I cannot edit the issue description myself, would it be possible to add them as todo-like items in the issue description so that we can mark them complete as make progress.

@raulcd raulcd modified the milestones: 16.0.0, 17.0.0 Apr 15, 2024
@raulcd raulcd modified the milestones: 17.0.0, 18.0.0 Jun 28, 2024
@raulcd raulcd modified the milestones: 18.0.0, 19.0.0 Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants