Replace long-deprecated getpass(3) call
AbandonedPublic
Actions

Authored by awilcox on Jan 31 2017, 6:30 PM.

Details

Reviewers: None

Summary

This modernises the kcheckpass utility to stop using the getpass(3) call, which was deprecated in SUSv2, removed from POSIX.1-2001, and deprecated in glibc 2.19. It replaces it with a fully standards compliant getdelim(3) call. This also avoids the need to strdup the password buffer and temporarily have two copies.

I didn't know how pedantic to make it; you could possibly want to check for password != NULL and memset-nul it out in the case of getdelim failure, since it could have read in a partial password but then received EOF before \n. I didn't think this case was very likely so I did not author such a check.

Test Plan

Tested against shadow backend and PAM backend.
Each backend was tested on x86_64, x86_32, and PowerPC.
Both glibc and musl libc were tested. FreeBSD testing is pending.

Diff Detail

Repository

R133 KScreenLocker

Lint

Lint Skipped

Unit

Unit Tests Skipped

awilcox updated this revision to Diff 10779.Jan 31 2017, 6:30 PM

awilcox retitled this revision from to Replace long-deprecated getpass(3) call.

awilcox updated this object.

awilcox edited the test plan for this revision. (Show Details)

awilcox set the repository for this revision to R133 KScreenLocker.

Restricted Application added a project: Plasma. · View Herald TranscriptJan 31 2017, 6:30 PM

Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

Out of interest: how did you stumble on that code? After all the usage in kscreenlocker should not enter the code path. KScreenlocker uses the conv_server approach. And IIRC there is no other usage of kcheckpass any more.

I didn't think this case was very likely so I did not author such a check.

We can think about how likely it is: this is code run on every system when the screen is unlocked. I do that ~10 times a day. Let's say a normal users does it once a day. Makes it 365 times a year. Let's assume we have a million users. That's 365 million times this code gets called per year. The unlikely event can get quite likely with large numbers ;-)

If you think there is a risk: better be pedantic in this case. On the other hand getdelim should return -1 in error case and then your method returns null. So in my book that's good enough error checking.

kscreenlocker-5.8.5/kcheckpass/kcheckpass.c
102	nitpck: coding style. Whitespace missing between if and (

I have fixed the style issue with no space between if and bracket.

Out of interest: how did you stumble on that code?

Using the musl libc, getpass is not defined unless you enable _GNU_SOURCE:

[ 75%] Building C object kcheckpass/CMakeFiles/kcheckpass.dir/kcheckpass.c.o
cd /usr/src/kde-plasma/kscreenlocker-5.8.5/work/kscreenlocker-5.8.5_build/kcheckpass && /usr/bin/gcc  -D_LARGEFILE64_SOURCE -D_XOPEN_SOURCE=600 -I/usr/src/kde-plasma/kscreenlocker-5.8.5/work/kscreenlocker-5.8.5_build/kcheckpass -I/usr/src/kde-plasma/kscreenlocker-5.8.5/work/kscreenlocker-5.8.5/kcheckpass -I/usr/src/kde-plasma/kscreenlocker-5.8.5/work/kscreenlocker-5.8.5_build   -DQT_NO_DEBUG -DNDEBUG -O2 -mlong-double-64 -ggdb -mcpu=G3 -fno-omit-frame-pointer  -std=iso9899:1990 -fno-common -Wall -Wextra -Wcast-align -Wchar-subscripts -Wformat-security -Wno-long-long -Wpointer-arith -Wundef -Wmissing-format-attribute -Wwrite-strings -Werror=implicit-function-declaration -std=gnu90 -fvisibility=hidden    -U_REENTRANT -o CMakeFiles/kcheckpass.dir/kcheckpass.c.o -c /usr/src/kde-plasma/kscreenlocker-5.8.5/work/kscreenlocker-5.8.5/kcheckpass/kcheckpass.c
/usr/src/kde-plasma/kscreenlocker-5.8.5/work/kscreenlocker-5.8.5/kcheckpass/kcheckpass.c: In function ‘conv_legacy’:
/usr/src/kde-plasma/kscreenlocker-5.8.5/work/kscreenlocker-5.8.5/kcheckpass/kcheckpass.c:105:10: error: implicit declaration of function ‘getpass’ [-Werror=implicit-function-declaration]
      p = getpass(prompt ? prompt : "Password: ");
          ^
/usr/src/kde-plasma/kscreenlocker-5.8.5/work/kscreenlocker-5.8.5/kcheckpass/kcheckpass.c:105:8: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
      p = getpass(prompt ? prompt : "Password: ");
        ^
cc1: some warnings being treated as errors
kcheckpass/CMakeFiles/kcheckpass.dir/build.make:62: recipe for target 'kcheckpass/CMakeFiles/kcheckpass.dir/kcheckpass.c.o' failed
make[2]: *** [kcheckpass/CMakeFiles/kcheckpass.dir/kcheckpass.c.o] Error 1
make[2]: Leaving directory '/usr/src/kde-plasma/kscreenlocker-5.8.5/work/kscreenlocker-5.8.5_build'
CMakeFiles/Makefile2:2042: recipe for target 'kcheckpass/CMakeFiles/kcheckpass.dir/all' failed
make[1]: *** [kcheckpass/CMakeFiles/kcheckpass.dir/all] Error 2

If you think there is a risk: better be pedantic in this case. On the other hand getdelim should return -1 in error case and then your method returns null. So in my book that's good enough error checking.

My concern was on the off-chance that getdelim reads a partial password but receives EOF before \n so it returns -1. But, looking at the standard, there is no safe way to read/write to the buffer if the return code is -1, so that is actually a moot point.

I just wanted to add that after a convoluted process (ripping all the Wayland checks out of CMakeLists.txt, and only building the kcheckpass target, since the screen locker itself does not even build at all...), I have tested that this change *does* work correctly on FreeBSD 10.3-RELEASE-p11.

Since a9c44a1 conv_legacy is no longer an option for kscreenlocker. This patch is no longer necessary as of Plasma 5.10.

Revision Contents
Changeset List

			Path	Packages
M			kscreenlocker-5.8.5/kcheckpass/kcheckpass.c (46 lines)

Diff 10782

View Options

kscreenlocker-5.8.5/kcheckpass/kcheckpass.c

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	10779		Jan 31 2017, 6:26 PM	★	★
Diff 2	10782		Jan 31 2017, 7:25 PM	★	★