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

Length of optimized c++ strings does not change when prefix is replaced #2524

Closed
Zaharid opened this issue Nov 27, 2017 · 2 comments
Closed
Labels
locked [bot] locked due to inactivity stale::closed [bot] closed after being marked as stale stale [bot] marked as stale due to inactivity

Comments

@Zaharid
Copy link

Zaharid commented Nov 27, 2017

Consider a piece of C++11 code like:

std::string get_profile_path() {
  char *env_path = std::getenv("NNPDF_PROFILE_PATH");
  if (env_path) {
    return env_path;
  }
  auto p = DEFAULT_NNPDF_PROFILE_PATH;
  return p;
}

where DEFAULT_NNPDF_PROFILE_PATH is some compile time constant that contains the conda prefix, and we hope to have replaced for the corresponding environment prefix.

This works, and the substitution indeed happens. However users end up seeing a path like:

'/Users/user/miniconda3/share/NNPDF/nnprofile.yaml\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\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'

with all the zeros. The reason is that an optimizing compiler may decide to build the std::string object at compile time and set its length to the length of the "placeholder" prefix, while not bothering to recompute it at runtime, from the position of the first \0. This then breaks in all sorts of ways (e.g. python raises an exception if \0s are passed as paths). The solution is to make a variable volatile:

std::string get_profile_path() {
  char *env_path = std::getenv("NNPDF_PROFILE_PATH");
  if (env_path) {
    return env_path;
  }
  volatile auto p = DEFAULT_NNPDF_PROFILE_PATH;
  return p;
}

to force the compiler to skip the optimization for that code.

I don't know if there is a better way of doing this. In particular it requires patching third party code. See:
Zaharid/conda-recipes@c17240a

I have seen this problem with both gcc on linux and clang on mac, when sufficiently recent versions are used.

@mingwandroid
Copy link
Contributor

This is a tricky one. I do not think we should consider trying to detect and modify std::strings.

In general software needs to be made to be relocatable. It's not difficult on the 3 major OSes to determine where the executable (or shared library) is located and to perform some path manipulation based upon that. This also avoids the need to mark those files as needing binary prefix replacement in their conda packages which means hardlinks can usually be used instead of copies.

On MSYS2 we had to occasionally patch traditionally Unix software to change them to do this dynamic lookup instead of expecting --prefix to be constant.

@github-actions
Copy link

Hi there, thank you for your contribution!

This issue has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further activity occurs.

If you would like this issue to remain open please:

  1. Verify that you can still reproduce the issue at hand
  2. Comment that the issue is still reproducible and include:
    - What OS and version you reproduced the issue on
    - What steps you followed to reproduce the issue

NOTE: If this issue was closed prematurely, please leave a comment.

Thanks!

@github-actions github-actions bot added the stale [bot] marked as stale due to inactivity label Sep 24, 2022
@github-actions github-actions bot added the stale::closed [bot] closed after being marked as stale label Oct 24, 2022
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Oct 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity stale::closed [bot] closed after being marked as stale stale [bot] marked as stale due to inactivity
Projects
None yet
Development

No branches or pull requests

2 participants