Feature/serialtube #90

JonathanBeverley · 2018-01-29T19:29:52Z

Add a class to handle serial connections.
There is a significant bugfix in lib/pwnlib/tubes/tube.rb, which is necessary for it to work properly.
I've bumped version to 1.0.2 (mostly so I could tell I was using it in testing.)

This change is

david942j · 2018-01-30T02:28:05Z

Thanks for contributing!

But please fix these issues before we start to review:

Don't bump the version, you can do it locally but don't commit it
Resolve conflicts
Add tests and ensure 100% coverage
- Beware of our tests run under three operating systems
Add documents
Follow STYLE.md, some issues I found in a quick review:
- Use first instead of [0]
- Sort the requiring
- Sort the dependencies in gemspec

Thanks again!

david942j · 2018-01-30T02:30:01Z

And it would be great if you file an issue to describe how the bug effects in tube.rb

… into feature/serialtube

…be one

david942j · 2018-06-09T03:30:32Z

lib/pwnlib/timer.rb

@@ -54,8 +54,9 @@ def countdown(timeout = nil)
      begin
        yield
      ensure
-        raise ::Pwnlib::Errors::TimeoutError unless active?
+        was_active = active?


Thanks for pointing out this,
it's fixed by #116

david942j · 2018-06-09T03:33:38Z

lib/pwnlib/tubes/tube.rb

@@ -159,7 +166,7 @@ def recvuntil(delims, drop: false, timeout: nil)
            while @timer.active?
              s = recv(1)

-              return '' if s.empty?


Is this change needed?

No. I raised it in issue #91, and it was classified as "notbug".

So please rollback it

david942j · 2018-06-09T03:36:07Z

lib/pwnlib/tubes/tube.rb

              data << c
            end
            data.slice!(0..-1)
-          ensure


Why this?
Original code seems correct

This might be a disagreement on what the function is supposed to do. In my opinion, it should not return until the timeout expires unless it has a match with the predicate. Just because a particular recv(1) didn't return anything doesn't mean that one .5s later won't.

Swap from rescue->raise is different. That's because we must not unrecv(data) if we match the predicate. With ensure, we always unrecv.

But recv_raw is a block call
The situation you mentioned should never happen

david942j · 2018-06-09T03:40:03Z

test/tubes/serialtube_test.rb

+
+class SerialTest < MiniTest::Test
+  include ::Pwnlib::Tubes
+


Is the serial tube intend to work on macOS?
If yes, let the tests pass on macOS (install socat)
If no, skip the test unless the environment is on Linux

And skip on Windows as well

I don't have access to a mac to test it on. It seems likely that it will work off-the-shelf.
How do I "install socat"?

rubyserial is supported on Windows, and so it's possible that serialtube will work there. I don't have a Windows/ruby setup in-place, nor do I have any idea how to test it there.

To install socat, simply add brew install socat in travis/install.sh#setup_osx

Is that possible to test serialtube on Windows? I said skip on Windows because the tests you added (socat ...) can't be supported on Windows.

If serialtube is supposed to support on Windows, maybe you should try another testing method to make it work on Windows as well.
Otherwise, just forget Windows supports and skip the tests.

david942j · 2018-06-09T03:43:31Z

lib/pwnlib/pwn.rb

@@ -13,6 +13,7 @@
 require 'pwnlib/reg_sort'
 require 'pwnlib/shellcraft/shellcraft'
 require 'pwnlib/tubes/sock'
+require 'pwnlib/tubes/serialtube'



Put this line before 'pwnlib/tubes/sock'

david942j · 2018-06-12T01:19:06Z

lib/pwnlib/tubes/serialtube.rb

+      #
+      # @!macro raise_eof
+      def recv_raw(numbytes)
+        raise EOFError if @conn.nil?


raise ::Pwnlib::Errors::EndOfTubeError

david942j · 2018-06-12T01:19:57Z

lib/pwnlib/tubes/serialtube.rb

+            return data
+          rescue RubySerial::Error
+            close
+            raise EOFError


david942j · 2018-06-12T01:20:48Z

lib/pwnlib/tubes/serialtube.rb

+      #
+      # @!macro raise_eof
+      def send_raw(data)
+        raise EOFError if @conn.nil?


david942j · 2018-06-12T01:20:56Z

lib/pwnlib/tubes/serialtube.rb

+          return @conn.write(data)
+        rescue RubySerial::Error
+          close
+          raise EOFError


david942j · 2018-06-12T01:23:42Z

lib/pwnlib/tubes/serialtube.rb

+
+      # Sets the +timeout+ to use for subsequent +recv_raw+ calls.
+      #
+      # @param [Integer] timeout


The type is Float?
And it's ok to not document private methods if there's nothing worth to be mentioned

david942j · 2018-06-12T01:31:06Z

lib/pwnlib/tubes/serialtube.rb

+      # @return [String]
+      #   A string containing read bytes.
+      #
+      # @!macro raise_eof


Yard doesn't support cross-file macro definition naturally
https://stackoverflow.com/questions/10324926/how-to-make-yard-macros-apply-to-multiple-files

david942j · 2018-06-12T01:34:30Z

lib/pwnlib/tubes/tube.rb

@@ -51,6 +51,9 @@ def initialize(timeout: nil)
      # @return [String]
      #   A string contains bytes received from the tube, or +''+ if a timeout occurred while
      #   waiting.
+      #
+      # @!macro raise_eof
+      # @!macro raise_timeout


Same as above

This one isn't cross-file.

david942j · 2018-06-12T01:36:08Z

lib/pwnlib/tubes/tube.rb

@@ -159,7 +166,7 @@ def recvuntil(delims, drop: false, timeout: nil)
            while @timer.active?
              s = recv(1)

-              return '' if s.empty?


So please rollback it

david942j · 2018-06-12T01:37:02Z

lib/pwnlib/tubes/tube.rb

              data << c
            end
            data.slice!(0..-1)
-          ensure


But recv_raw is a block call
The situation you mentioned should never happen

david942j · 2018-06-12T05:55:01Z

lib/pwnlib/tubes/tube.rb

@@ -105,9 +108,13 @@ def recvpred(timeout: nil)
              data << c
            end
            data.slice!(0..-1)
-          ensure
+          # rubocop:disable Style/RescueStandardError


And this must be changed back to ensure

I think I finally get what I misunderstood about this code, but I'll raise it as an issue so it can be discussed properly.

david942j · 2018-06-12T05:58:07Z

lib/pwnlib/tubes/serialtube.rb

+      # @param [Integer] baudrate
+      #   Baud rate.
+      # @param [Boolean] convert_newlines
+      #   If +true+, will convert local +context.newline+ to +"\\r\\n"+ for remote


I guess no escape needed here?
+"\r\n"+ should be enough

And this sentence seems not complete

What I try it without the double escape, in the .html files generated by YARD, I see
If true, will MARKER convert local context.newline to "rn" for remote
I copied the syntax from tube.rb, which does the same thing. Note that in @example blocks the rules are different.

david942j · 2018-06-12T06:01:21Z

lib/pwnlib/tubes/serialtube.rb

+              break unless data.empty?
+              sleep 0.1
+            end
+            # TODO: should we reverse @convert_newlines here?


TODO(username)

…ley/pwntools-ruby into JonathanBeverley-feature/serialtube

david942j · 2018-06-13T02:02:00Z

lib/pwnlib/tubes/serialtube.rb

+
+module Pwnlib
+  module Tubes
+    # Serial Connections


change here to

# @!macro [new] raise_eof # @raise [Pwnlib::Errors::EndOfTubeError] # If the request is not satisfied when all data is received. # Serial Connections class SerialTube < Tube

otherwise YARD will have wrong result

david942j · 2018-06-13T02:03:43Z

lib/pwnlib/tubes/serialtube.rb

+            # XXX(JonathanBeverey): should we reverse @convert_newlines here?
+            return data
+          rescue RubySerial::Error
+            close


Current tests don't cover this rescue
add tests and ensure 100% coverage

I can't see any way to do this. open(2) checks for all the same errors as read(2). Nothing I've done will cause an error in the right place, without writing bad code.

If you know a way the test code can break Ruby's encapsulation model and directly invoke SerialTube.conn.close(), that could do it. But I really don't want to write a method to allow that, and honestly, if I did, I would then tighten up the error checking around @conn and close off that avenue as well.

I found a way.

david942j · 2018-06-13T02:03:50Z

lib/pwnlib/tubes/serialtube.rb

+        begin
+          return @conn.write(data)
+        rescue RubySerial::Error
+          close


david942j · 2018-06-15T11:12:30Z

Seems the tests sometimes hang forever
Any idea?

JonathanBeverley · 2018-07-05T04:06:36Z

I've tried chasing it down, but I can't reproduce it locally. It's probably happening because we're stuck in the Open3.popen3() block. Before I got the ensure working properly something similar would happen when an exception tried to break out of that block, but popen3 would wait for socat to complete before allowing it.

…bes::Process

…ley/pwntools-ruby into JonathanBeverley-feature/serialtube

david942j · 2018-08-22T15:42:13Z

With while true; do bundle exec rake; done I can reproduce the hanging forever case.
It stucks on Serial#read, i.e. the read syscall

With quickly review I know the rubyserial gem do set VMIN=0 for a non-blocking IO, so this should not happen.
Trying to figure out why.

david942j · 2018-08-23T17:26:26Z

Got it.
The main reason is a race condition between rubyserial and socat,
they both need to set (different) attributes on the same tty device.
Just add one line to make sure socat have finished its setup fix the issue: 764ada5#diff-8dfbabae43dfe4e708d49fc1f86db83dR36

Now dealing with another failed case.. https://travis-ci.org/peter50216/pwntools-ruby/jobs/419754587

david942j · 2018-08-24T14:47:34Z

@JonathanBeverley I fixed the issues
While I don't have permission to push those fixes to the forked repo, could you push them to your branch?

The fixes contained in two commits: bda6f8b and 764ada5 (plz ignore the debug messages)

JonathanBeverley · 2018-08-24T17:28:48Z

done.

david942j · 2018-08-25T03:19:08Z

Sorry those commits contain debug code..
please checked-out the changes in test/asm_test.rb and uncomment (# install_keystone_from_source) in travis/install.sh

JonathanBeverley and others added 5 commits January 17, 2018 00:33

add serialtube, up version 1.0.2

b3fe465

auto require SocketTube as part of pwn

ab1d1ee

BUGFIX: can't return from within a timer loop, must next

5cd3a53

Rubyserial doesn't handle timeouts. Working around.

e1a674a

style fix: no arguments to next

20e6a38

JonathanBeverley added 16 commits February 1, 2018 19:22

revert version to 1.0.1

31b57b3

add serialtube_test; some cleanup of serialtube

7dfa4f4

Merge branch 'master' into feature/serialtube

7cfc0fc

serialtube function comments

ad30289

Handle timeouts well enough

f1930b7

style fixes

55cc048

old updates to sock_test

ba86f2c

Merge remote-tracking branch 'refs/remotes/origin/feature/serialtube'…

cbffd90

… into feature/serialtube

make code uglier to make rubocop happy

39a3fbc

rename SerialTube internal timer to avoid interfering with generic Tu…

1da19e8

…be one

revert accidental change to sock_test.rb

3a3661b

Bugfix: in case of timeout, we need to deactivate the timer

7b84223

Bugfix: in recv_pred, we only want to unrecv if there was an error

94c3b08

Bugfix: recv_raw should return early if there is ANY data

4772e6a

Fixing issues in testcode

78a21a4

reduce timeouts for faster testing; eliminate a rubocop disable

867b892

david942j reviewed Jun 9, 2018

View reviewed changes

JonathanBeverley added 2 commits June 9, 2018 00:26

sort requires

b6077ae

multiplatform testing: install socat for mac, skip for windows

ffdee6f

david942j requested changes Jun 12, 2018

View reviewed changes

make requested changes

6b61cb1

david942j requested changes Jun 12, 2018

View reviewed changes

JonathanBeverley and others added 3 commits June 12, 2018 10:50

revert ensure->rescue; clarify some comments

ebfa490

Merge branch 'feature/serialtube' of https://github.com/JonathanBever…

4fc138e

…ley/pwntools-ruby into JonathanBeverley-feature/serialtube

fix doc

b04dbff

david942j requested changes Jun 13, 2018

View reviewed changes

JonathanBeverley added 2 commits June 13, 2018 00:43

Improve test coverage slightly

4ac3f86

break encapsulation to test recv_raw raise

79a8b33

JonathanBeverley and others added 7 commits July 13, 2018 14:09

bugfix: random_string(length) would sometimes return a shorter string

33b2d21

Merge branch 'master' into feature/serialtube

f6a724d

explicitly reference ::Process to about name conflict with Pwnlib::Tu…

cf5f22e

…bes::Process

Merge branch 'feature/serialtube' of https://github.com/JonathanBever…

3add469

…ley/pwntools-ruby into JonathanBeverley-feature/serialtube

verbose

910c0dc

Don't build keystone..

55a87c9

recv is suspicious

589dd09

guess I resolved the bug

764ada5

david942j added 2 commits August 24, 2018 02:06

Add wait after kill

bda6f8b

Clean debug messages

0f68f83

remove debug code

8257d80

david942j approved these changes Aug 25, 2018

View reviewed changes

david942j merged commit c01a52e into peter50216:master Aug 25, 2018

Feature/serialtube #90

Feature/serialtube #90

Conversation

JonathanBeverley commented Jan 29, 2018 • edited by peter50216 Loading

david942j commented Jan 30, 2018 • edited Loading

david942j commented Jan 30, 2018

Choose a reason for hiding this comment

david942j Jun 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david942j commented Jun 15, 2018

JonathanBeverley commented Jul 5, 2018

david942j commented Aug 22, 2018 • edited Loading

david942j commented Aug 23, 2018 • edited Loading

david942j commented Aug 24, 2018

JonathanBeverley commented Aug 24, 2018

david942j commented Aug 25, 2018 • edited Loading

JonathanBeverley commented Jan 29, 2018 •

edited by peter50216

Loading

david942j commented Jan 30, 2018 •

edited

Loading

david942j Jun 9, 2018 •

edited

Loading

david942j commented Aug 22, 2018 •

edited

Loading

david942j commented Aug 23, 2018 •

edited

Loading

david942j commented Aug 25, 2018 •

edited

Loading