Adding wo*y commands using clipboard instead of [val]

[cr]

When working with streaming ciphers, being able to XOR memory regions inside radare is super nifty. This implementation uses the clipboard as XOR key which is repeated when it is shorter than blocksize.

[radare]

You can already do this with the wox command. not in the clipboard.. and there are several other "encryption" algorithms and checksums that can be applied to a buffer, so i would prefer not to have more dupplicated commands, unless we follow the same schema.

For example. having yo subcommands to mimic the wo ones. and share as much code as possible between them.

[cr]

You can already do this with the wox command.

wox makes you specify the key on the command line which is not feasible with a 1K stream cipher. The main motivation for yX was to be able to XOR two memory regions.

My first instinct was to extend the wox command accordingly, but you end up introducing one or two awkward length parameters. Using the clipboard instead makes it elegant to simply yank a key from somewhere, then XOR it where needed.

[radare]

oh i get your point. Using the clipboard as a place to store the key. My suggestion would be to add an y suffix to the wo. commands and take the clipboard as key instead of parsing the argument.

> woxy

[cr]

An elegant patch just turned ugly, but I may have made it. make tests runs without errors, but if the integration test here fails again, I don't know what's causing it, because it seems I can't read its output here.

[cr]

I couldn't bring myself to rewrite the arg parsing for the woE command.

[cr]

I am getting second thoughts here: Wouldn't it be more sensible to fall back to clipboard when no [val] is given instead of introducing the wo*y class of commands?

[cr]

I have a version ready that falls back to clipboard when no [val] is given. Let me know if you prefer that, and I'll push it here first thing in the morning.

[Maijin]

Hey dat looks cool stuff, have you checked #4280 and #4254 as it is a bit related?

[radare]

this shuold be alphabetically sorted

[cr]

It's been a mess before, so it was hard to make out the right spot. I tried to work surgically. Shouldn't a cleanup here be part of a different pull request?

[radare]

r2 have a huge codebase and we try to have as much testcases as possible in order to not break stuff, because one of the main premises is the continuous refactoring, mainly because most of the stuff that is implemented in r2 is done "quickly" because is a feature someone needs for something NOW. but there are always better ways to do stuff, so that's where the refactoring enters the game. you can do this alphabetically sorting thing in the same PR, there's no real issue in sorting out a text array that will be processed by humans.

[cr]

Thanks for the explanation. I'm sorry. I didn't mean to drive you into defense. :/

I will push along a separate commit in this pull request.

[radare]

Into defense? Dont take me wrong. Im not trying to be offensive or anything. Just trying to give some feedback on every new contributor just to discuss better way to handle this.

Thats a very nice addition :) take a beer and chill a bit ;)

Ill merge after lunch

Thx

On 07 Apr 2016, at 14:49, Christiane Ruetten [email protected] wrote:

In libr/core/cmd_write.c:

    "wor"," [val]", ">>= shift right",
    "wol"," [val]","<<= shift left",
    "wo2"," [val]","2=  2 byte endian swap",
    "wo4"," [val]", "4=  4 byte endian swap",

•   "wo*y", "", "same as their counterparts above, but using clipboard instead of [val]",
  

  Thanks for the explanation. I'm sorry. I didn't mean to drive you into defense. :/

I will push along a separate commit in this pull request.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[cr]

No offense taken at all! I was just slightly worried that you felt offended because you started justifying radare code quality there.

[radare]

Sorry for the late reply, its been a crazy day yesterday. Can you address the comments and update the PR? looks good to merge

[cr]

@radare, please comment on #4533 (comment) . I much prefer that solution.

[cr]

TBH, I have never gotten in that much trouble for not touching the existing code. I am thankful that you pointed out the mistakes I was about to leave in place, but I also feel that we might be better off to separate my feature work from what seems to be a much-needed audit/rewrite of the existing code.

[cr]

Woah, someone merged this PR into master before we were done with the review. :(

[cr]

I am closing this PR. There's not much sense in trying to fix the broken merge on top of this.

[radare]

wtf?

you dont have to fix any merge and you can push -f in pr'd branches perfectly. github handles this

[radare]

ok moved to #4548

[cr]

This PR was poisoned because I had force-pushed into this branch without realizing that it had already been merged.