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

SIGABRT with glibc program_invocation_short_name #92

Closed
markhindley opened this Issue Nov 5, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@markhindley
Copy link
Contributor

markhindley commented Nov 5, 2018

Hello,

I am packaging v239.1 for Devuan and Debian. When running the "elogind -D" I get SIGABRT in elogind_free_program_name(). The issue appears to be that with this glibc (version in Debian unstable is 2.27) program_invocation_short_name overlaps with program_invocation_short_name rather than pointing to a separate buffer as the code expects.

There is a related issue in src/basic/process-util.c where, under the same conditions, rename_process() changes program_invocation_name with strncopy which can leave program_invocation_short_name as meaningless.

Mark

@Yamakuzure

This comment has been minimized.

Copy link
Collaborator

Yamakuzure commented Nov 7, 2018

That is rather unique. Both variables are protected by __GLIBC__ defined by glibc. The SIGABRT suggests that __GLIBC__ was not defined when building elogind. How can this be?

Oh, hang on!

That was changed to have it checked by meson. If it is detected that these variables are provided, HAVE_PROGRAM_INVOCATION_NAME is defined as 1 and they do not get declared.

The test snipped is here:

have_pisn = cc.links('''

Could you please compile and run it on your machine and post the return code? Does it return 0 or 1?

Edit: rename_process() is used when forking to make the child meaningful. ;-)

@markhindley

This comment has been minimized.

Copy link
Contributor Author

markhindley commented Nov 7, 2018

@Yamakuzure

This comment has been minimized.

Copy link
Collaborator

Yamakuzure commented Nov 8, 2018

Well, glibc defines those variables and their implementation details are irrelevant.

However, I noticed that the new guarding was not only incomplete, it was also meant well but made too complex.

I'd advise you to postpone packaging elogind until next week when I will release v239.2 ; which has many fixes.

Yamakuzure added a commit that referenced this issue Nov 8, 2018

Prep v239: Fix handlinjg of program_invocation[_short]_name (#92)
The new detection works, but the consequences weren't enforced
strictly enough.

This lead to SIGABRT on glibc-2.27 systems, when elogind detected
that bot variables where different and tried to free them.
But program_invocation_short_name is just a pointer into to long name
memory, so freeing it is leading to a crash.

The resolution is to fully use the new detection: If the used libc
provides both, we do not care about anything any more, as the used
libc is bound to handle everything correctly.

Bug: #92
Closes: #92
Signed-Off-By: Sven Eden <sven.eden@prydeworx.com>

Yamakuzure added a commit that referenced this issue Nov 8, 2018

Prep v239.2: Fix handlinjg of program_invocation[_short]_name (#92)
The new detection works, but the consequences weren't enforced
strictly enough.

This lead to SIGABRT on glibc-2.27 systems, when elogind detected
that both variables where different and tried to free them.
But program_invocation_short_name is just a pointer into to long name
memory, so freeing it is leading to a crash.

The resolution is to fully use the new detection: If the used libc
provides both, we do not care about anything any more, as the used
libc is bound to handle everything correctly.

Bug: #92
Closes: #92
Signed-Off-By: Sven Eden <sven.eden@prydeworx.com>

@Yamakuzure Yamakuzure self-assigned this Nov 8, 2018

@Yamakuzure Yamakuzure added bug and removed need information labels Nov 8, 2018

@Yamakuzure Yamakuzure closed this in 03c5c2a Nov 8, 2018

Yamakuzure added a commit that referenced this issue Nov 8, 2018

Prep v239.2: Fix handling of program_invocation[_short]_name (#92)
The new detection works, but the consequences weren't enforced
strictly enough.

This lead to SIGABRT on glibc-2.27 systems, when elogind detected
that both variables where different and tried to free them.
But program_invocation_short_name is just a pointer into to long name
memory, so freeing it is leading to a crash.

The resolution is to fully use the new detection: If the used libc
provides both, we do not care about anything any more, as the used
libc is bound to handle everything correctly.

Bug: #92
Closes: #92
Signed-Off-By: Sven Eden <sven.eden@prydeworx.com>

Yamakuzure added a commit that referenced this issue Nov 8, 2018

Prep v239.2: Fix handling of program_invocation[_short]_name (#92)
The new detection works, but the consequences weren't enforced
strictly enough.

This lead to SIGABRT on glibc-2.27 systems, when elogind detected
that both variables where different and tried to free them.
But program_invocation_short_name is just a pointer into to long name
memory, so freeing it is leading to a crash.

The resolution is to fully use the new detection: If the used libc
provides both, we do not care about anything any more, as the used
libc is bound to handle everything correctly.

Bug: #92
Closes: #92
Signed-Off-By: Sven Eden <sven.eden@prydeworx.com>
@markhindley

This comment has been minimized.

Copy link
Contributor Author

markhindley commented Nov 11, 2018

@Yamakuzure

This comment has been minimized.

Copy link
Collaborator

Yamakuzure commented Nov 12, 2018

@markhindley : Yes, now you mentioned it, my dmesg shows only the PID, and no name.

... let me see if I can do something about that...

@Yamakuzure Yamakuzure reopened this Nov 12, 2018

Yamakuzure added a commit that referenced this issue Nov 12, 2018

Prep v239.2: Handle program_invocation_short_name more properly (#92)
With glibc, at least v2.28, program_invocation_short_name is not a
string of its own, but simply points into program_invocation_name.

When elogind is forked, the fork process gets a new name. Upstream
has decided to change to long version, which is the correct thing
to do. Unfortunately program_invocation_short_name is not adapted,
and all further log messages using it might print anything.

This patch manipulates program_invocation_short_name if its adress
lies within program_invocation_name and its original length.

Bug: #92
Closes: #92
Signed-off-by: Sven Eden <sven.eden@prydeworx.com>
@Yamakuzure

This comment has been minimized.

Copy link
Collaborator

Yamakuzure commented Nov 12, 2018

@markhindley : Should be fixed in v239-stable now.

At least my dmesg now shows:

[  +1,297556] elogind-daemon[9007]: New session c2 of user sddm.
[  +5,909004] elogind-daemon[9007]: New session 2 of user sed.
[ +23,762704] elogind-daemon[9007]: Removed session 2.
[  +2,814813] elogind-daemon[9007]: New session 3 of user sed.
[  +0,013002] elogind-daemon[9007]: Removed session c2.
@markhindley

This comment has been minimized.

Copy link
Contributor Author

markhindley commented Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment