Re: Review Request 124413: Enable PAM opening KWallet again

[Martin Klapetek]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/
---

(Updated July 27, 2015, 11:06 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.


Changes
---

Submitted with commit aead424f00f91f141e8a14d0871ac97602242f11 by Martin 
Klapetek to branch master.


Repository: kwallet


Description
---

This brings back Alex's patch in commit 
f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be opened 
by PAM if kwallet-pam is present.

http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14


Diffs
-

  src/runtime/kwalletd/main.cpp b4e3837 

Diff: https://git.reviewboard.kde.org/r/124413/diff/


Testing
---

Logged in, KWallet does not ask for password anymore.


Thanks,

Martin Klapetek

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124413: Enable PAM opening KWallet again

[Valentin Rusu]

> On July 27, 2015, 8:02 a.m., Valentin Rusu wrote:
> > Ship It!

and thanks for this.


- Valentin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/#review83014
---


On July 21, 2015, 5:27 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124413/
> ---
> 
> (Updated July 21, 2015, 5:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> This brings back Alex's patch in commit 
> f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
> opened by PAM if kwallet-pam is present.
> 
> http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/main.cpp b4e3837 
> 
> Diff: https://git.reviewboard.kde.org/r/124413/diff/
> 
> 
> Testing
> ---
> 
> Logged in, KWallet does not ask for password anymore.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124413: Enable PAM opening KWallet again

[Valentin Rusu]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/#review83014
---

Ship it!


Ship It!

- Valentin Rusu


On July 21, 2015, 5:27 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124413/
> ---
> 
> (Updated July 21, 2015, 5:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> This brings back Alex's patch in commit 
> f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
> opened by PAM if kwallet-pam is present.
> 
> http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/main.cpp b4e3837 
> 
> Diff: https://git.reviewboard.kde.org/r/124413/diff/
> 
> 
> Testing
> ---
> 
> Logged in, KWallet does not ask for password anymore.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124413: Enable PAM opening KWallet again

[Lamarque Souza]

> On July 21, 2015, 3:57 p.m., Lamarque Souza wrote:
> > src/runtime/kwalletd/main.cpp, line 113
> > 
> >
> > You should use strncmp instead of strcmp.
> 
> Martin Klapetek wrote:
> Why would you think? The whole string is being compared, what good would 
> strncmp do in here?
> 
> Lamarque Souza wrote:
> I was thinking about using something like
> 
> if (strncmp(argv[x], "--pam-login", sizeof("--pam-login")))
> 
> It's a general rule not use use strcmp in security sensitive code since 
> it only stops to compare characters when it finds a null character. If no 
> such character exists in the compared string then you will have a buffer 
> overflow. Since this is an argv string it probably contains a null byte, so 
> the "should" and not "have to". It is just recomendation, you can drop it if 
> you wish.
> 
> Stefan Brüns wrote:
> But "--pam-login" is null terminated, so you will compare at most 
> sizeof("--pam-login") bytes anyway.

Yeah, you're right.


- Lamarque


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/#review82770
---


On July 21, 2015, 5:27 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124413/
> ---
> 
> (Updated July 21, 2015, 5:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> This brings back Alex's patch in commit 
> f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
> opened by PAM if kwallet-pam is present.
> 
> http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/main.cpp b4e3837 
> 
> Diff: https://git.reviewboard.kde.org/r/124413/diff/
> 
> 
> Testing
> ---
> 
> Logged in, KWallet does not ask for password anymore.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124413: Enable PAM opening KWallet again

[Stefan Brüns]

> On July 21, 2015, 3:57 p.m., Lamarque Souza wrote:
> > src/runtime/kwalletd/main.cpp, line 113
> > 
> >
> > You should use strncmp instead of strcmp.
> 
> Martin Klapetek wrote:
> Why would you think? The whole string is being compared, what good would 
> strncmp do in here?
> 
> Lamarque Souza wrote:
> I was thinking about using something like
> 
> if (strncmp(argv[x], "--pam-login", sizeof("--pam-login")))
> 
> It's a general rule not use use strcmp in security sensitive code since 
> it only stops to compare characters when it finds a null character. If no 
> such character exists in the compared string then you will have a buffer 
> overflow. Since this is an argv string it probably contains a null byte, so 
> the "should" and not "have to". It is just recomendation, you can drop it if 
> you wish.

But "--pam-login" is null terminated, so you will compare at most 
sizeof("--pam-login") bytes anyway.


- Stefan


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/#review82770
---


On July 21, 2015, 5:27 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124413/
> ---
> 
> (Updated July 21, 2015, 5:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> This brings back Alex's patch in commit 
> f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
> opened by PAM if kwallet-pam is present.
> 
> http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/main.cpp b4e3837 
> 
> Diff: https://git.reviewboard.kde.org/r/124413/diff/
> 
> 
> Testing
> ---
> 
> Logged in, KWallet does not ask for password anymore.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124413: Enable PAM opening KWallet again

[Lamarque Souza]

> On July 21, 2015, 3:57 p.m., Lamarque Souza wrote:
> > src/runtime/kwalletd/main.cpp, line 113
> > 
> >
> > You should use strncmp instead of strcmp.
> 
> Martin Klapetek wrote:
> Why would you think? The whole string is being compared, what good would 
> strncmp do in here?

I was thinking about using something like

if (strncmp(argv[x], "--pam-login", sizeof("--pam-login")))

It's a general rule not use use strcmp in security sensitive code since it only 
stops to compare characters when it finds a null character. If no such 
character exists in the compared string then you will have a buffer overflow. 
Since this is an argv string it probably contains a null byte, so the "should" 
and not "have to". It is just recomendation, you can drop it if you wish.


> On July 21, 2015, 3:57 p.m., Lamarque Souza wrote:
> > src/runtime/kwalletd/main.cpp, line 126
> > 
> >
> > You should use strtol() instead of atoi() for better error checking. 
> > atoi() does no error checking at all.
> 
> Martin Klapetek wrote:
> ...but the code does not check for errors (and does not need to)?

Actually, the error checking is done in line 135, so you can drop this one.


- Lamarque


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/#review82770
---


On July 21, 2015, 5:27 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124413/
> ---
> 
> (Updated July 21, 2015, 5:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> This brings back Alex's patch in commit 
> f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
> opened by PAM if kwallet-pam is present.
> 
> http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/main.cpp b4e3837 
> 
> Diff: https://git.reviewboard.kde.org/r/124413/diff/
> 
> 
> Testing
> ---
> 
> Logged in, KWallet does not ask for password anymore.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124413: Enable PAM opening KWallet again

[Martin Klapetek]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/
---

(Updated July 21, 2015, 7:27 p.m.)


Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.


Changes
---

Fix issues

(as a side I'd like to point out that this patch was already reviewed once in 
https://git.reviewboard.kde.org/r/116555/ and I merely just cherry-pick the 
patch from kde-runtime)


Repository: kwallet


Description
---

This brings back Alex's patch in commit 
f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be opened 
by PAM if kwallet-pam is present.

http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14


Diffs (updated)
-

  src/runtime/kwalletd/main.cpp b4e3837 

Diff: https://git.reviewboard.kde.org/r/124413/diff/


Testing
---

Logged in, KWallet does not ask for password anymore.


Thanks,

Martin Klapetek

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124413: Enable PAM opening KWallet again

[Martin Klapetek]

> On July 21, 2015, 5:57 p.m., Lamarque Souza wrote:
> > src/runtime/kwalletd/main.cpp, line 144
> > 
> >
> > You should deallocate hash here if it is not null, otherwise you have a 
> > memory leak.
> 
> Martin Klapetek wrote:
> I don't think so - the hash is init'ed as NULL; it is set by 
> waitForHash() which either returns NULL (so hash stays NULL) or returns some 
> data, at which point the code will not reach this line (can someone double 
> check).

Ah no I read it wrong, hash can be not null in here. I'll deallocate it.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/#review82770
---


On July 21, 2015, 5:52 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124413/
> ---
> 
> (Updated July 21, 2015, 5:52 p.m.)
> 
> 
> Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> This brings back Alex's patch in commit 
> f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
> opened by PAM if kwallet-pam is present.
> 
> http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/main.cpp b4e3837 
> 
> Diff: https://git.reviewboard.kde.org/r/124413/diff/
> 
> 
> Testing
> ---
> 
> Logged in, KWallet does not ask for password anymore.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124413: Enable PAM opening KWallet again

[Martin Klapetek]

> On July 21, 2015, 5:57 p.m., Lamarque Souza wrote:
> > src/runtime/kwalletd/main.cpp, line 113
> > 
> >
> > You should use strncmp instead of strcmp.

Why would you think? The whole string is being compared, what good would 
strncmp do in here?


> On July 21, 2015, 5:57 p.m., Lamarque Souza wrote:
> > src/runtime/kwalletd/main.cpp, line 126
> > 
> >
> > You should use strtol() instead of atoi() for better error checking. 
> > atoi() does no error checking at all.

...but the code does not check for errors (and does not need to)?


> On July 21, 2015, 5:57 p.m., Lamarque Souza wrote:
> > src/runtime/kwalletd/main.cpp, line 144
> > 
> >
> > You should deallocate hash here if it is not null, otherwise you have a 
> > memory leak.

I don't think so - the hash is init'ed as NULL; it is set by waitForHash() 
which either returns NULL (so hash stays NULL) or returns some data, at which 
point the code will not reach this line (can someone double check).


- Martin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/#review82770
---


On July 21, 2015, 5:52 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124413/
> ---
> 
> (Updated July 21, 2015, 5:52 p.m.)
> 
> 
> Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> This brings back Alex's patch in commit 
> f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
> opened by PAM if kwallet-pam is present.
> 
> http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/main.cpp b4e3837 
> 
> Diff: https://git.reviewboard.kde.org/r/124413/diff/
> 
> 
> Testing
> ---
> 
> Logged in, KWallet does not ask for password anymore.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124413: Enable PAM opening KWallet again

[Lamarque Souza]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/#review82770
---



src/runtime/kwalletd/main.cpp (line 113)


You should use strncmp instead of strcmp.



src/runtime/kwalletd/main.cpp (line 126)


You should use strtol() instead of atoi() for better error checking. atoi() 
does no error checking at all.



src/runtime/kwalletd/main.cpp (line 129)


environment



src/runtime/kwalletd/main.cpp (line 144)


You should deallocate hash here if it is not null, otherwise you have a 
memory leak.


- Lamarque Souza


On July 21, 2015, 3:52 p.m., Martin Klapetek wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124413/
> ---
> 
> (Updated July 21, 2015, 3:52 p.m.)
> 
> 
> Review request for KDE Frameworks, Àlex Fiestas and Valentin Rusu.
> 
> 
> Repository: kwallet
> 
> 
> Description
> ---
> 
> This brings back Alex's patch in commit 
> f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be 
> opened by PAM if kwallet-pam is present.
> 
> http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14
> 
> 
> Diffs
> -
> 
>   src/runtime/kwalletd/main.cpp b4e3837 
> 
> Diff: https://git.reviewboard.kde.org/r/124413/diff/
> 
> 
> Testing
> ---
> 
> Logged in, KWallet does not ask for password anymore.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 124413: Enable PAM opening KWallet again

[Martin Klapetek]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124413/
---

Review request for KDE Frameworks and Valentin Rusu.


Repository: kwallet


Description
---

This brings back Alex's patch in commit 
f2fe3e75b4ba12d0f99aa09327059a1865891b14 [1] which allows KWallet to be opened 
by PAM if kwallet-pam is present.

http://quickgit.kde.org/?p=kde-runtime.git&a=commit&h=f2fe3e75b4ba12d0f99aa09327059a1865891b14


Diffs
-

  src/runtime/kwalletd/main.cpp b4e3837 

Diff: https://git.reviewboard.kde.org/r/124413/diff/


Testing
---

Logged in, KWallet does not ask for password anymore.


Thanks,

Martin Klapetek

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel