TDE
/
tdepim
6
0
Fork 1
Code Issues 23 Pull Requests 5 Releases Wiki Activity
Labels Milestones

Adapted to new KPasswordEdit::password() signature. This relates to bug 2961. #12

Merged
SlavekB merged 1 commits from bug/2961 into master 5 years ago
Conversation 1 Commits 1 Files Changed 12
SlavekB commented 5 years ago
Owner

I add another piece to the bug 2961 puzzle. I did not test the result. By the way, I did not find out if anyone else uses the finished() signal from certmanager/lib/ui/passphrasedialog.h – whether changing the parameter may cause additional problems.

I add another piece to the bug 2961 puzzle. I did not test the result. By the way, I did not find out if anyone else uses the `finished()` signal from [`certmanager/lib/ui/passphrasedialog.h`](src/branch/master/certmanager/lib/ui/passphrasedialog.h#L72) – whether changing the parameter may cause additional problems.
SlavekB added this to the R14.1.0 release milestone 5 years ago
SlavekB added the PR/rfc label 5 years ago
MicheleC reviewed 5 years ago
certmanager/lib/ui/passphrasedialog.cpp Outdated
MicheleC commented 5 years ago
Owner

IMO, const modifier is not required. Gives more freedom to user of the function without requiring cast.
In tdelibs and many other places, TQString is used.

IMO, const modifier is not required. Gives more freedom to user of the function without requiring cast. In tdelibs and many other places, TQString is used.
MicheleC reviewed 5 years ago
certmanager/lib/ui/passphrasedialog.h Outdated
MicheleC commented 5 years ago
Owner

IMO, const modifier is not required. Gives more freedom to user of the function without requiring cast.
In tdelibs and many other places, TQString is used.

IMO, const modifier is not required. Gives more freedom to user of the function without requiring cast. In tdelibs and many other places, TQString is used.
MicheleC reviewed 5 years ago
certmanager/lib/ui/passphrasedialog.h Outdated
MicheleC commented 5 years ago
Owner

const TQString&

const TQString&
MicheleC reviewed 5 years ago
libkpgp/kpgp.cpp Outdated
MicheleC commented 5 years ago
Owner

Being an object, passphrase will be deallocated when it leaves its scope. There should be no need for explicitly replace it with an empty TQString.

As sidenote, freeMem logic may no longer be necessary, perhaps?

Being an object, passphrase will be deallocated when it leaves its scope. There should be no need for explicitly replace it with an empty TQString. As sidenote, freeMem logic may no longer be necessary, perhaps?
MicheleC reviewed 5 years ago
libkpgp/kpgp.cpp
MicheleC commented 5 years ago
Owner

We are not using malloc any more in the body. So a simpler version of the function could be used, IMO.

To consider if "havePassPhrase" is still required.

bool Module::setPassPhrase(const TQString& aPass)
{
   passphrase = aPass;
   havePassPhrase = true;
   return true;
}
We are not using malloc any more in the body. So a simpler version of the function could be used, IMO. To consider if "havePassPhrase" is still required. ``` bool Module::setPassPhrase(const TQString& aPass) { passphrase = aPass; havePassPhrase = true; return true; } ```
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.cpp Outdated
MicheleC commented 5 years ago
Owner

"const TQString &passphrase" looks better 😉

"const TQString &passphrase" looks better :wink:
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.cpp Outdated
MicheleC commented 5 years ago
Owner

should use isEmpty() instead of "isNull()". This will check for "" strings as well.

should use isEmpty() instead of "isNull()". This will check for "" strings as well.
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.cpp Outdated
MicheleC commented 5 years ago
Owner

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.cpp Outdated
MicheleC commented 5 years ago
Owner

“const TQString &passphrase” looks better 😉

“const TQString &passphrase” looks better :wink:
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.cpp Outdated
MicheleC commented 5 years ago
Owner

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.cpp Outdated
MicheleC commented 5 years ago
Owner

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.cpp Outdated
MicheleC commented 5 years ago
Owner

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.cpp Outdated
MicheleC commented 5 years ago
Owner

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.

should use isEmpty() instead of “isNull()”. This will check for “” strings as well.
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.h Outdated
MicheleC commented 5 years ago
Owner

const TQString& = TQString::null) seems more appropriate

const TQString& = TQString::null) seems more appropriate
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.h Outdated
MicheleC commented 5 years ago
Owner

const TQString& = TQString::null) seems more appropriate

const TQString& = TQString::null) seems more appropriate
MicheleC reviewed 5 years ago
libkpgp/kpgpbase.h Outdated
MicheleC commented 5 years ago
Owner

Here and in all other 14 changes in this file (till the end)

“const TQString &passphrase” looks better

Here and in all other 14 changes in this file (till the end) “const TQString &passphrase” looks better
MicheleC reviewed 5 years ago
libkpgp/kpgpbase2.cpp Outdated
MicheleC commented 5 years ago
Owner

Here and all similar changes till end of PR

“const TQString &passphrase” looks better

Here and all similar changes till end of PR “const TQString &passphrase” looks better
MicheleC reviewed 5 years ago
libkpgp/kpgpbase2.cpp Outdated
MicheleC commented 5 years ago
Owner

here and all following changes where isNull() is used:

should use isEmtpy() to check also for ""

here and all following changes where isNull() is used: should use isEmtpy() to check also for ""
MicheleC reviewed 5 years ago
libkpgp/kpgpui.cpp Outdated
MicheleC commented 5 years ago
Owner

IMO, const modifier is not required. Gives more freedom to user of the function without requiring cast. In tdelibs and many other places, TQString is used.

IMO, const modifier is not required. Gives more freedom to user of the function without requiring cast. In tdelibs and many other places, TQString is used.
MicheleC reviewed 5 years ago
libkpgp/kpgpui.h Outdated
MicheleC commented 5 years ago
Owner

IMO, const modifier is not required. Gives more freedom to user of the function without requiring cast. In tdelibs and many other places, TQString is used.

IMO, const modifier is not required. Gives more freedom to user of the function without requiring cast. In tdelibs and many other places, TQString is used.
MicheleC reviewed 5 years ago
MicheleC left a comment
Owner

see comments mixed in the diff view. Minor things anyway.

see comments mixed in the diff view. Minor things anyway.
SlavekB reviewed 5 years ago
certmanager/lib/ui/passphrasedialog.cpp Outdated
SlavekB commented 5 years ago
Poster
Owner

Yes, you are right – because we do not return the pointer to existing data, but the new TQString, const has no reason.

Yes, you are right – because we do not return the pointer to existing data, but the new TQString, const has no reason.
SlavekB reviewed 5 years ago
certmanager/lib/ui/passphrasedialog.h Outdated
SlavekB commented 5 years ago
Poster
Owner

Very good note, thank you.

Very good note, thank you.
SlavekB reviewed 5 years ago
libkpgp/kpgp.cpp Outdated
SlavekB commented 5 years ago
Poster
Owner

I believe that two situations have been addressed here:

  1. Targeted cleaning of the original memory to ensure that the password is not left forgotten in memory.
  2. Shorten the previous length of the string => moreover, there was intentional use of isNull instead of isEmpty. Here we could use truncate instead of the new TQString.
I believe that two situations have been addressed here: 1. Targeted cleaning of the original memory to ensure that the password is not left forgotten in memory. 2. Shorten the previous length of the string => moreover, there was intentional use of isNull instead of isEmpty. Here we could use truncate instead of the new TQString.
SlavekB reviewed 5 years ago
libkpgp/kpgp.cpp
SlavekB commented 5 years ago
Poster
Owner

As I mentioned above – I think wipePassPhrase is primarily security-oriented here to make sure that the previous memory contents is destroyed.

As I mentioned above – I think wipePassPhrase is primarily security-oriented here to make sure that the previous memory contents is destroyed.
SlavekB added the PR/wip label 5 years ago
MicheleC reviewed 5 years ago
libkpgp/kpgp.cpp Outdated
MicheleC commented 5 years ago
Owner

ah, you are right, I had not see this in the full context. My bad :-(

ah, you are right, I had not see this in the full context. My bad :-(
MicheleC reviewed 5 years ago
libkpgp/kpgp.cpp
MicheleC commented 5 years ago
Owner

yeah, wipePassPhrase() is required, to be honest I missed that line. Anyhow I think we should use isEmpty() instead of isNull(). If aPass =="" it makes no sense to say that we have a password 😉

yeah, wipePassPhrase() is required, to be honest I missed that line. Anyhow I think we should use isEmpty() instead of isNull(). If aPass =="" it makes no sense to say that we have a password :wink:
SlavekB commented 5 years ago
Poster
Owner

Comments have been incorporated along with several other changes in the same sense – see 6492b716b3.

Comments have been incorporated along with several other changes in the same sense – see 6492b716b3.
SlavekB removed the PR/wip label 5 years ago
MicheleC approved these changes 5 years ago
MicheleC left a comment
Owner

Looks ready to merge 😄

Looks ready to merge :smile:
MicheleC removed the PR/rfc label 5 years ago
SlavekB closed this pull request 5 years ago
SlavekB deleted branch bug/2961 5 years ago
The pull request has been merged as 6492b716b3.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
Labels
Apply labels
Clear labels
GE/need-info
General - need additional info from contributor PR/keep-branch
Pull request - do not delete branch after merging PR/not-ok
Pull request - need fixing PR/rfc
Pull request - request for comments PR/update-trans
Pull request - update to translation files needed PR/wip
Pull request - work in progress RS/R14.0.x
Related to R14.0.x series RS/R14.1.x
Related to R14.1.x series SL/critical
Severity level - critical SL/major
Severity level - major SL/minor
Severity level - minor SL/normal
Severity level - normal SL/regression
Severity level - regression from previous version SL/trivial
Severity level - trivial SL/wishlist
Severity level - wishlist request ST/duplicate
Status - duplicate of another issue ST/invalid
Status - invalid report ST/notourproblem
Status - not our problem ST/rejected
Status - rejected ST/wontfix
Status - won't fix ST/worksforme
Status - works for me, unable to reproduce
No Label GE/need-info PR/keep-branch PR/not-ok PR/rfc PR/update-trans PR/wip RS/R14.0.x RS/R14.1.x SL/critical SL/major SL/minor SL/normal SL/regression SL/trivial SL/wishlist ST/duplicate ST/invalid ST/notourproblem ST/rejected ST/wontfix ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.1.0 release
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdepim#12
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'bug/2961'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes