Skip to content

[lipstick] serialize/hide devicelock plugin arguments#199

Open
rainisto wants to merge 1 commit into
nemomobile:masterfrom
rainisto:pipe
Open

[lipstick] serialize/hide devicelock plugin arguments#199
rainisto wants to merge 1 commit into
nemomobile:masterfrom
rainisto:pipe

Conversation

@rainisto

Copy link
Copy Markdown
Contributor

Do not merge untill devicelock encryption plugin pull request have been merged.
This pull request serializes commandline arguments so it can be piped into encryption plugin's stdin.

EDIT: new devicelock plugin has now been merged to devel. So its when this receives LGTM then someone with merge rights can merge this in.

Comment thread src/devicelock/devicelock.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation.

Comment thread src/devicelock/devicelock.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be &.

@rainisto rainisto force-pushed the pipe branch 3 times, most recently from b375e05 to 50b228e Compare October 2, 2014 12:33
Comment thread src/devicelock/devicelock.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably call QByteArray::append(char ch)?

@rainisto

Copy link
Copy Markdown
Contributor Author

Rebased and updated against review comments. And verified functionality against latest version of devicelock plugin in devel.

Comment thread src/devicelock/devicelock.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document (or fix) that limit of 127 characters per argument. Also add a check for it here, or the overflow behavior is a potential problem.

@rainisto rainisto force-pushed the pipe branch 2 times, most recently from 50abc31 to b01ed8a Compare December 13, 2014 20:46
@thp

thp commented Jan 29, 2015

Copy link
Copy Markdown
Contributor

Should this be merged or dropped? @rainisto @special

@rainisto

Copy link
Copy Markdown
Contributor Author

Well it depends, merged if we want to fix/hide plugin arguments from current plugin and dropped if plugin rewrite will use different approach. I haven't seen any plans how the new/rewritten plugin will work so I don't know if it will want to reuse this approach or not. Personally I would vote for merge as it improves security on current device, but if rewrite takes different approach, then I would keep this open until @special or someone doing the rewritten plugin comments how they plan to pass the parameters to the plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants