eCryptfs

Merge lp://staging/~wlxing/ecryptfs/lp1695767 into lp://staging/ecryptfs

lp1695767
Merge into trunk

Proposed by Jason Xing on 2017-06-05

Status:	Merged
Merge reported by:	Tyler Hicks
Merged at revision:	not available
Proposed branch:	lp://staging/~wlxing/ecryptfs/lp1695767
Merge into:	lp://staging/ecryptfs
Diff against target:	15 lines (+5/-0) 1 file modified src/libecryptfs/decision_graph.c (+5/-0)
To merge this branch:	bzr merge lp://staging/~wlxing/ecryptfs/lp1695767
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Tyler Hicks	2017-06-05	Approve on 2017-06-08
Jason Xing (community)		Needs Resubmitting on 2017-06-07
Review via email: mp+325079@code.staging.launchpad.net

Description of the change

Adding just six lines to test whether the value of "num_transitions" is set to zero or not because it stands for how many options user could choose, we can then easily prevent infinite loop and prompt user with error messages. That is to say, if set to zero, that means that eCryptfs goes wrong because there is no selection for user to choose.

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2017-06-07:

Hi Jason - Thanks for the patch!

After reviewing the function that you modified, I think the check for
node->num_transitions being zero can be done much higher. Around line
465, where node->num_transitions is already being checked for being
equal to one, should work fine and then you can drop the call to free
the "prompt" string.

Also, a few comments on patch style...

1) Please put the description of the patch into the commit message
   itself instead of into the pull request description. Someone
   researching why this change was made will typically look into the bzr
   log history but may not look at the Launchpad pull request.

2) The correct syntax for referencing a Launchpad bug is (LP: #1695767).
The syntax used in the commit message includes an unnecessary "lp".

3) The patch includes a space character between the third and fourth tab
characters on the line that rc is being assigned.

review: Needs Fixing

lp://staging/~wlxing/ecryptfs/lp1695767 updated on 2017-06-07

888. By Jason Xing on 2017-06-07

Reproduce case:
1) User doesn't install openssl.
2) Run ecryptfs-manager and make selection 3.
It goes in the infinite loop...which means whatever selection you enter next cannot end this command or alter your option.

Explanation for the patch:
Adding several lines to take such a case into consideration, through testing whether "num_transitions" is set to zero or not we could prevent the infinite loop. If it is set to zero, it will return -EINVAL because "num_transitions" means how many options this command could show to users and zero means that user has no option to choose. (LP: #1695767)

Revision history for this message

Jason Xing (wlxing) wrote on 2017-06-07:

Hell Tyler,

Thanks for your instructions!!!

I move those codes much higher and delete "free(prompt);" because it has not allocated memory for "prompt". Also, I delete the space before "rc = ..." line. BTW, I'm wondering how you found the space between the third and the forth tab?

Please review the patch one more time, thanks again.

review: Needs Resubmitting

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2017-06-08:

This looks good. Thanks!

I adjusted the error message and printed it to stderr before committing the fix. Thanks again!

review: Approve