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

process-util: don't use overly large buffer to store process command line #11527

Merged
merged 1 commit into from
Jan 26, 2019

Conversation

msekletar
Copy link
Contributor

Allocate new string as a return value and free our "scratch pad"
buffer that is potentially much larger than needed (up to
_SC_ARG_MAX).

Fixes #11502

Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be possible to be a bit smarter when the strdup is done. E.g. in the kernel thread case, we allocate a string of precisely the right length, so we could avoid the copying without any loss.

I don't think this is a the whole solution. It seems the "leak" is growing and growing, so just making smaller allocations cannot be the whole answer, but this looks like a step in the right direction. So let's merge this, as it should make things much nicer, and look for further improvements later.

src/basic/process-util.c Outdated Show resolved Hide resolved
@msekletar
Copy link
Contributor Author

I don't think this is a the whole solution. It seems the "leak" is growing and growing, so just making smaller allocations cannot be the whole answer, but this looks like a step in the right direction. So let's merge this, as it should make things much nicer, and look for further improvements later.

Maybe we should track how big is each ClientContext entry and have limits on how much memory overall can cache consume.

@keszybz
Copy link
Member

keszybz commented Jan 22, 2019

Maybe we should track how big is each ClientContext entry and have limits on how much memory overall can cache consume.

I'm working on a patch right now.

@msekletar
Copy link
Contributor Author

@keszybz I've updated my branch. PTAL.

@keszybz keszybz added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jan 22, 2019
@keszybz keszybz mentioned this pull request Jan 22, 2019
@msekletar
Copy link
Contributor Author

Ubuntu CI failures are most likely unrelated and seem to be due to packaging issues,

E: There were unauthenticated packages and -y was used without --allow-unauthenticated
autopkgtest: WARNING: Test dependencies are unsatisfiable - calling apt install on test deps directly for further data about failing dependencies in test logs
boot-smoke           FAIL badpkg

@poettering
Copy link
Member

The allocation of the sysconf size is still in there. The upper loop should be reworked to do GREEDY_REALLOC() really, so that the buffer increases exponentially in size as needed.

@keszybz
Copy link
Member

keszybz commented Jan 22, 2019

GREEDY_REALLOC()

I thought about this, but there's an argument for doing this as in the current patch: those strings will be kept in a cache, potentially for a long time, so it is quite worthwhile to minimize the allocation. And if we are going to do the strdup at the end anyway, and the maximum size is fixed anyway and not that large, then it's OK to not use greedy_realloc.

@poettering
Copy link
Member

GREEDY_REALLOC()

I thought about this, but there's an argument for doing this as in the current patch: those strings will be kept in a cache, potentially for a long time, so it is quite worthwhile to minimize the allocation. And if we are going to do the strdup at the end anyway, and the maximum size is fixed anyway and not that large, then it's OK to not use greedy_realloc.

it appears to me the right approach is to use greedy_realloc() initially, and then use realloc() at the end to give up the space not actually needed. realloc() is pretty ok for giving up unneeded memory, much better i think than copying yourself

@msekletar msekletar removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jan 26, 2019
…line

Allocate new string as a return value and free our "scratch pad"
buffer that is potentially much larger than needed (up to
_SC_ARG_MAX).

Fixes systemd#11502
@poettering poettering added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jan 26, 2019
@poettering poettering merged commit eb1ec48 into systemd:master Jan 26, 2019
buildroot-auto-update pushed a commit to buildroot/buildroot that referenced this pull request May 30, 2019
Fixes #11911

Systemd-journald would leak memory when recording process info.  Add
patch files from upstream systemd.  Note that the patch from 2d5d2e0cc5
was taken as well in order to make the needed commit apply cleanly.

Bug report: systemd/systemd#11502
Accepted patch: systemd/systemd#11527

Signed-off-by: Jonah Petri <jonah@petri.us>
[Peter: add bz reference, add s-o-b to patches, drop numbering]
Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed util-lib
Development

Successfully merging this pull request may close these issues.

None yet

3 participants