D13100: do not use buffered file IO
This revision was automatically updated to reflect the committed changes. Closed by commit R107:fcbcf0f1bdfa: do not use buffered file IO (authored by dakon). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D13100?vs=43538=44535#toc REPOSITORY R107 KWallet PAM Integration CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13100?vs=43538=44535 REVISION DETAIL https://phabricator.kde.org/D13100 AFFECTED FILES pam_kwallet.c To: dakon, aacid Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13100: do not use buffered file IO
aacid added inline comments. INLINE COMMENTS > dakon wrote in pam_kwallet.c:696 > While this is "just an int", my personal taste is very much against using > const on a file descriptor as this is not how this really works. I don't know. It is how it really works. Think of as a handle value, you're promising not to assign that handle value to point somewhere else, not that you won't change the value it points to. Why i want this const? Because when you see that you can be sure that the rest of the file there's no other fd = open() or similar, and thus you're sure that the rest of the operations in this function that use fd are over "path". REVISION DETAIL https://phabricator.kde.org/D13100 To: dakon, aacid Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13100: do not use buffered file IO
dakon added inline comments. INLINE COMMENTS > aacid wrote in pam_kwallet.c:696 > const While this is "just an int", my personal taste is very much against using const on a file descriptor as this is not how this really works. I don't know. > aacid wrote in pam_kwallet.c:704 > const ACK REVISION DETAIL https://phabricator.kde.org/D13100 To: dakon, aacid Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13100: do not use buffered file IO
aacid accepted this revision.
aacid added a comment.
This revision is now accepted and ready to land.
Please make me const-happy before commiting :)
INLINE COMMENTS
> pam_kwallet.c:696
> char *salt = gcry_random_bytes(KWALLET_PAM_SALTSIZE,
> GCRY_STRONG_RANDOM);
> -FILE *fd = fopen(path, "w");
> +int fd = open(path, O_CREAT | O_WRONLY | O_TRUNC | O_CLOEXEC, 0600);
>
const
> pam_kwallet.c:704
>
> -fwrite(salt, KWALLET_PAM_SALTSIZE, 1, fd);
> -fclose(fd);
> +ssize_t wlen = write(fd, salt, KWALLET_PAM_SALTSIZE);
> +close(fd);
const
> pam_kwallet.c:755
>
> -FILE *fd = fopen(path, "r");
> -if (fd == NULL) {
> +int fd = open(path, O_RDONLY | O_CLOEXEC);
> +if (fd == -1) {
const
> pam_kwallet.c:764
> char salt[KWALLET_PAM_SALTSIZE] = {};
> -const int bytesRead = fread(salt, 1, KWALLET_PAM_SALTSIZE, fd);
> -fclose(fd);
> +ssize_t bytesRead = read(fd, salt, KWALLET_PAM_SALTSIZE);
> +close(fd);
const
REVISION DETAIL
https://phabricator.kde.org/D13100
To: dakon, aacid
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D13100: do not use buffered file IO
dakon updated this revision to Diff 43538. dakon added a comment. O_CREAT was missing, as well as the file mode. O_CLOEXEC is available for a long time, there should be no need for the compat define. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13100?vs=34833=43538 REVISION DETAIL https://phabricator.kde.org/D13100 AFFECTED FILES pam_kwallet.c To: dakon, aacid Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13100: do not use buffered file IO
aacid added a comment. have you tried this? because i have just tried it and it failed. Aren't you missing a O_CREAT ? REPOSITORY R107 KWallet PAM Integration REVISION DETAIL https://phabricator.kde.org/D13100 To: dakon, aacid Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D13100: do not use buffered file IO
dakon created this revision. dakon added a reviewer: aacid. Restricted Application added a project: Plasma. Restricted Application added a subscriber: plasma-devel. dakon requested review of this revision. REVISION SUMMARY This is not necessary here. Additionally we can use O_CLOEXEC to make sure the file descriptors are not leaked by accident. REPOSITORY R107 KWallet PAM Integration REVISION DETAIL https://phabricator.kde.org/D13100 AFFECTED FILES pam_kwallet.c To: dakon, aacid Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
