Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3 Changes: use variable in make, change tty handling, and remove undefined behavior wrt memcpy #8

Closed
wants to merge 5 commits into from

Conversation

codyps
Copy link
Contributor

@codyps codyps commented Jun 18, 2012

  • Notes on the tty handling changes: I didn't understand why you where doing open("/dev/tty", O_RDONLY), so the changes may be wrong.
    The goal was to allow me to pipe a password into tcplay via standard in. isatty is used to determine whether to prompt the user or simply read from input. I suppose this could cause issues if the user (for some reason) lacks a tty. Let me know what you think.
  • In the makefile, I use variables in the test and clean target instead of hardcoded programs.
  • tcplay memcpy's to the same buffer when doing cipher chaning (in = out, in the cipher chaining loop). This eventually gets to a function which memcpy's in to out. memcpy is not technically allowed to do this (pointers must point to independed memory regions), so I check for that case and remove it.
    Thoughts: It would probably be cleaner to remove the memcpy between buffers entirely by somehow hoisting it out of this function. I honestly didn't look to hard at this option yet.

@bwalex
Copy link
Owner

bwalex commented Jun 21, 2012

Sorry - I haven't had time to review this yet. I'll hopefully have time this weekend.

Thanks!
Alex

@bwalex
Copy link
Owner

bwalex commented Jun 21, 2012

The reason I am accessing /dev/tty instead of just relying on stdin is pretty much to avoid hijacking of stdin - someone putting a wrapper around tcplay that logs passphrases.

To pass in passphrases non-interactively the best thing to do would be to use the API, although I see how that is not convenient at all for a shell script or so.

If you separate out the changes to the tty business, I'll merge the rest straight ahead.

Cheers,
Alex

@codyps
Copy link
Contributor Author

codyps commented Jun 22, 2012

Wouldn't hijacking the controlling terminal also be possible in any case where hijacking stdin is possible?

@bwalex
Copy link
Owner

bwalex commented Nov 16, 2012

After having another look at all of this, I've decided it's not really worth trying to handle weird cases when the advantages outweigh the disadvantages like they do in this case.

I've committed your code with some minor changes. Thanks, and sorry for the delay!

@bwalex bwalex closed this Nov 16, 2012
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.

2 participants