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

Prefix replacement in osx-64 breaks build as it causes null bytes in the middle of paths #1351

Closed
geoffreygarrett opened this issue Jun 28, 2020 · 4 comments

Comments

@geoffreygarrett
Copy link

I apologise in advance if this I'm missing something fundamental here, but this has caused a large roadblock for me, and the process of building the package and dependencies is time consuming. I have attempted to reproduce this in a much smaller example, but have been unable to. I must mention that I have very limited experience with clang, which is what I believe is the most likely cause at this point.

Issue

The issue is logged here when debugging our feedstock, where the linux build passes and the osx fails. It shows that prefix replacement is leaving null bytes in replaced prefix paths. When attempting to pass these strings onto a legacy function which I'd prefer not to modify, it is passed on as:

$PREFIX/resource\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00/spice_kernels/

which is interpreted as a null terminated string

$PREFIX/resource

Path variable code trace

The linux build passes without this problem however, which is where I feel I'm a little out of my depth due to my lack of experience with clang. I attempted to use the boost::filesystem::path type in hopes that it would differ from using strings, as I saw @isuruf mention in an archived gitter log there would be issues when returning the prefix as compile time constant std::strings during prefix replacement. Below is the trace through our code:

  1. Tudat_RESOURCE_DIR is configured during cmake install in the config.hpp.in.

#define Tudat_RESOURCE_DIR "@CMAKE_INSTALL_PREFIX@/resource"

  1. RESOURCE_FOLDER defined as macro of Tudat_RESOURCE_DIR in tudat/resource/resource.h.

#define RESOURCES_FOLDER Tudat_RESOURCE_DIR

  1. Path concatenation using += operator with boost::filesystem::path type
static inline fs::path get_spice_kernels_path() {
        return (get_resources_path() += fs::path(SPICE_KERNELS));
}
  1. Path is converted to std::string to work with legacy code with minimal refractoring in tudat/paths.hpp.
static inline std::string getSpiceKernelPath() {
        return get_spice_kernels_path().string();
}
  1. String is converted to c string and passed to a routine from a c-library.
//! Load a Spice kernel.
void loadSpiceKernelInTudat( const std::string& fileName )
{
    furnsh_c(  fileName.c_str( ) );
}
  1. Additionally, the path getters are wrapped here in Python using Pybind11, which is called to test for null bytes:
m.def("get_tudat_path", &tudat::paths::get_tudat_path);

Attempt to fix (trace step 3)

Attempted to set fixed lengths to each path variable as char in tudat/resource/resource.h, in hopes that the strcat would deal with the null terminated string in the concatenation. Still results in failure for only the osx build [full log]:

=================================== FAILURES ===================================
______________________________ test_no_null_bytes ______________________________
    def test_no_null_bytes():
        """ For osx
        - https://github.com/tudat-team/tudatpy-feedstock/issues/1
        - https://gitter.im/conda-forge/conda-forge.github.io/archives/2019/01/22
        """
>       assert '\x00' not in kernel.get_tudat_path()
E       AssertionError: assert '\x00' not in '/Users/trav...urce\x00\x00'
E         '\x00' is contained here:
E           $PREFIX/resource
E         ?                                   

I am aware that they still get converted to strings in tudat/paths.hpp. I have not had time to attempt completely removing the std::string type altogether from the tudat project. I apologise if this issue is trivial. It's been a little difficult as I am unable to test the intermediate tudat package, as the prefix path in the test environment exceeds the maximum of 255 chars for our cspice dependency (furnsh_c routine).

Initial failure Linux & osx build logs on CircleCI & TravisCI:

@geoffreygarrett geoffreygarrett changed the title Prefix replacement in osx-64 breaks build as it contains null bytes in the middle of paths Prefix replacement in osx-64 breaks build as it causes null bytes in the middle of paths Jun 28, 2020
@jakirkham
Copy link
Member

Would move this issue to conda-build since this is an issue with building packages. All conda-smithy does is configure repos with recipes to build on CI and deploy.

With that said, could try disabling prefix replacement selectively or generally in packages where it’s cause issues.

@beckermr
Copy link
Member

Agreed. If @jjhelmus wants to move the issue, that’s great, otherwise we should close it.

@geoffreygarrett
Copy link
Author

geoffreygarrett commented Jun 29, 2020

Seems to be conda/conda-build#1674. Specifically conda/conda-build#1674 (comment). As it's still open there, I'll close it here.

I've only seen this problem with clangxx (on osx) and not with gxx. It seems that clangxx is compiling string literals directly into std::string objects, such that the constructor of std::string is not executed with an argument of type char[N].

That effectively means that any string literal in C++ source code has to be a C-style null-terminated string, no matter how it is compiled.

@jjhelmus
Copy link
Contributor

wants to move the issue

AFAIK GitHub does not allow issues to be moved between organization 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants