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

Python 3.4 stream readline uses bytes instead of string #58

Closed
wants to merge 1 commit into from
Closed

Python 3.4 stream readline uses bytes instead of string #58

wants to merge 1 commit into from

Conversation

Dutchy-
Copy link
Contributor

@Dutchy- Dutchy- commented May 21, 2015

In my previous pull request, I only fix the obvious crash. However, there was more. I'm not 100% sure of the problem and the solution, but the current code works for me.

In python 3.4, the readline used for streams seems to return an empty byte array instead of an empty string. Without this, it keeps looping for streams.

My fix simply replaces '' with bytes(), however I'm not convinced this works for python versions other than 3.4. We'll have the build bot check it out first. If this doesn't work for the other versions, I'm not sure I can fix it, I might need some help.

My theory is that this was never caught because all the test cases run from files instead of from an actual nmap process.

My context and use case was https://groups.google.com/forum/#!msg/home-assistant-dev/hKcH32MzQvs/kF98gmFCIUQJ and I'm hoping for a new libnmap release soon :)

@Dutchy-
Copy link
Contributor Author

Dutchy- commented May 21, 2015

Never mind, this is wrong. Consider this a bug report, but my fix is not correct...

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.47% when pulling f537836 on Dutchy-:master into 496b547 on savon-noir:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.47% when pulling f537836 on Dutchy-:master into 496b547 on savon-noir:master.

@Dutchy-
Copy link
Contributor Author

Dutchy- commented May 21, 2015

Ok, I've really fixed this now. This is 100% confirmed working in python 3.4, but I have no idea about other versions. Please realise that your build bot seems to be not working, because it accepted both my previous 'fixes'.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.47% when pulling 8b70bc7 on Dutchy-:master into 496b547 on savon-noir:master.

@Dutchy-
Copy link
Contributor Author

Dutchy- commented May 22, 2015

I've made a gist to illustrate the problem:

https://gist.github.com/Dutchy-/c87b1c27b9c08fb0323c

There are several solutions:

  1. Always open files using byte mode, ie. open('file.txt', 'rb')
  2. Supply a temporary file to subprocess.Popen as described in http://stackoverflow.com/a/10134899

One of those may help in keeping things more compatible than they are now.

@savon-noir
Copy link
Owner

i'll check this out asap.

@savon-noir
Copy link
Owner

Hello, this fix gives error in py3.8:

Traceback (most recent call last):
File "proc_nmap_like.py", line 53, in
report = do_scan("127.0.0.1", "-sV")
File "proc_nmap_like.py", line 12, in do_scan
rc = nmproc.run()
File "/home/ronald/pydev/lib/python3.8/site-packages/libnmap/process.py", line 315, in run
self.__stdout += streamline.decode('utf-8')
AttributeError: 'str' object has no attribute 'decode'

@savon-noir
Copy link
Owner

savon-noir commented Nov 28, 2020

but the issue got fixed in: https://github.com/savon-noir/python-libnmap/pull/68/files thanks to both of you @Dutchy- @rcarrillo (sorry guys for that late update, i'm in a full review/clean-up process of this lib :)

@savon-noir savon-noir closed this Nov 28, 2020
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.

None yet

3 participants