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

Protect against --args being used twice #474

Closed
wants to merge 1 commit into from

Conversation

mwleeds
Copy link
Contributor

@mwleeds mwleeds commented Feb 18, 2022

Found by coverity CID 1471678 in flatpak

Found by coverity CID 1471678 in flatpak
@mwleeds
Copy link
Contributor Author

mwleeds commented Feb 18, 2022

Looks like the CI failed because there's a unit test that uses --args more than once:

+ printf '%s--dir\0/tmp/hello/world2\0' ''
+ printf '%s--dir\0/tmp/hello/world3\0' ''
+ /home/runner/work/bubblewrap/bubblewrap/_build/test-bwrap --bind / / --tmpfs /tmp --args 3 --args 4 --args 5 /bin/sh -c 'test -d /tmp/hello/world && test -d /tmp/hello/world2 && test -d /tmp/hello/world3'
bwrap: --args can only be used once

But I still think there's a bug here: opt_args_data only gets freed once in main() whereas it gets assigned to n times, n == number of times --args was specified.

@mwleeds
Copy link
Contributor Author

mwleeds commented Feb 18, 2022

On the other hand it's a memory leak on exit that's not likely to be huge so not sure how important it is.

@smcv
Copy link
Collaborator

smcv commented Feb 18, 2022

I still think there's a bug here

Quite possibly: see #209, #224, #230, #237, #426, #444.

On the other hand it's a memory leak on exit

As long as it's only O(n) in the number of arguments, I don't think there is any real impact to leaking up to a few K of memory.

@mwleeds
Copy link
Contributor Author

mwleeds commented Feb 18, 2022

Oh if there are already various issues about this I'll close this one.

@mwleeds mwleeds closed this Feb 18, 2022
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