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

shared: use memdup over stdndupa #92

Closed
wants to merge 1 commit into from

Conversation

evelikov
Copy link
Collaborator

@evelikov evelikov commented Sep 2, 2024

The stdndupa has a couple of gotchas:

  • allocates memory on stack via alloca(3)... where we pass it a user-provided string in at least one instance
  • it's a GNU extension missing on musl and bionic

The mkdir_p() function is not a hot path, so using heap allocation is perfectly fine. Swap the stdndupa for our local helper memdup.

The stdndupa has a couple of gotchas:
 - allocates memory on stack via alloca(3)... where we pass it a
   user-provided string in at least one instance
 - it's a GNU extension missing on musl and bionic

The mkdir_p() function is not a hot path, so using heap allocation is
perfectly fine. Swap the stdndupa for our local helper memdup.

Signed-off-by: Emil Velikov <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Sep 2, 2024
The stdndupa has a couple of gotchas:
 - allocates memory on stack via alloca(3)... where we pass it a
   user-provided string in at least one instance
 - it's a GNU extension missing on musl and bionic

The mkdir_p() function is not a hot path, so using heap allocation is
perfectly fine. Swap the stdndupa for our local helper memdup.

Signed-off-by: Emil Velikov <[email protected]>
Link: #92
Signed-off-by: Lucas De Marchi <[email protected]>
@lucasdemarchi
Copy link
Contributor

Applied, thanks

@evelikov evelikov deleted the rm-strndupa branch September 2, 2024 19:13
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

Successfully merging this pull request may close these issues.

2 participants