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

g++ fpermissive compilation error for strdupa on musl when buiding from source #15729

Closed
xhebox opened this issue Jun 23, 2022 · 5 comments
Closed
Labels
help wanted Someone outside the Bazel team could own this P3 We're not considering working on this, but happy to review a PR. (No assignee) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: support / not a bug (process)

Comments

@xhebox
Copy link

xhebox commented Jun 23, 2022

Description of the bug:

g++ will report errors as follows:

In file included from src/main/tools/linux-sandbox-pid1.cc:34:
src/main/tools/linux-sandbox-pid1.cc: In function ‘int CreateTarget(const char*, bool)’:
src/main/tools/linux-sandbox-pid1.cc:149:28: error: invalid conversion from ‘void*’ to ‘char*’ [-fpermissive]
  149 |   if (CreateTarget(dirname(strdupa(path)), true) < 0) {
      |                            ^
      |                            |
      |                            void*
In file included from src/main/tools/linux-sandbox-pid1.cc:34:
/include/string.h:33:15: note:   initializing argument 1 of ‘char* strcpy(char*, const char*)’
   33 | char *strcpy (char *__restrict, const char *__restrict);
      |               ^~~~~~~~~~~~~~~~
In file included from src/main/tools/linux-sandbox-pid1.cc:67:
src/main/tools/linux-sandbox-pid1.cc:150:36: error: invalid conversion from ‘void*’ to ‘char*’ [-fpermissive]
  150 |     DIE("CreateTarget %s", dirname(strdupa(path)));
      |                                    ^
      |                                    |
      |                                    void*
./src/main/tools/logging.h:36:51: note: in definition of macro ‘DIE’
   36 |     fprintf(stderr, __FILE__ ":" S__LINE__ ": \"" __VA_ARGS__); \
      |                                                   ^~~~~~~~~~~
In file included from src/main/tools/linux-sandbox-pid1.cc:34:
/include/string.h:33:15: note:   initializing argument 1 of ‘char* strcpy(char*, const char*)’
   33 | char *strcpy (char *__restrict, const char *__restrict);

I've sent patches to musl, ref this. Despite whether musl will accept the patch, I agreed with the author of musl that using strdupa is probably not a good idea. And we have other replacement.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Build bazel 5.2.0 on musl by:

curl -O -L https://github.com/bazelbuild/bazel/releases/download/5.2.0/bazel-5.2.0-dist.zip
7z x bazel-5.2.0.zip
EXTRA_BAZEL_ARGS="--tool_java_runtime_version=local_jdk" ./compile.sh

You could try this on an docker alpine linux image.

Which operating system are you running Bazel on?

linux with musl libc

What is the output of bazel info release?

No response

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@sgowroji
Copy link
Member

Hello @xhebox, Could you please provide complete steps to reproduce the above issue at our end. Thanks!

@xhebox
Copy link
Author

xhebox commented Jun 23, 2022

Hello @xhebox, Could you please provide complete steps to reproduce the above issue at our end. Thanks!

Edited, it really needs nothing more than normal building instructions. You could try this on alpine linux with docker. I am on my own custom linux distro with musl libc.

@sgowroji sgowroji added type: support / not a bug (process) untriaged team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website and removed more data needed labels Jun 23, 2022
@graywolf-at-work
Copy link
Contributor

I will send a patch removing the strdupa, since there is exactly one usage in whole of bazel.

@meteorcloudy meteorcloudy added P3 We're not considering working on this, but happy to review a PR. (No assignee) help wanted Someone outside the Bazel team could own this and removed untriaged labels Jun 28, 2022
graywolf-at-work pushed a commit to graywolf-at-work/bazel that referenced this issue Jun 29, 2022
Strdupa has potential to be unsafe thanks to the possibly unbound stack
usage. It also generates warnings when compiled on musl. This commit
therefore replaces it with properly checked heap allocation using
strdup.

Fixes bazelbuild#15729
@Wyverald
Copy link
Member

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jun 29, 2022
@ckolli5
Copy link

ckolli5 commented Jun 29, 2022

@bazel-io fork 5.3.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jun 29, 2022
ckolli5 added a commit that referenced this issue Jul 2, 2022
Strdupa has potential to be unsafe thanks to the possibly unbound stack
usage. It also generates warnings when compiled on musl. This commit
therefore replaces it with properly checked heap allocation using
strdup.

Fixes #15729

Closes #15763.

PiperOrigin-RevId: 458440234
Change-Id: I8c8574f654295086f767b4fc4ca6fc1e59097beb

Co-authored-by: Tomas Volf <tomas.volf@showmax.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Someone outside the Bazel team could own this P3 We're not considering working on this, but happy to review a PR. (No assignee) team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website type: support / not a bug (process)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants