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

Mssql script fixes #2784

Closed
wants to merge 3 commits into from
Closed

Conversation

johnjaylward
Copy link

This PR fixes 2 issues I ran into while trying to debug connection issues with a legacy SQL2000 server.

First commit corrects the return type expectation for a call made to Helper.GetDiscoveredInstances. The actual return is a single value, while the original expected return by the caller was a tuple. I corrected the caller to use only the single value.

The second commit fixes an issue I saw where the SqlServer returned no rows of data. The loop was being entered even though the pos var was already past the length of the data.

@correctmeifimwrong33
Copy link

Any reason this is not being merged?

Copy link

@nnposter nnposter left a comment

Choose a reason for hiding this comment

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

There is a more straightforward way how to accomplish a test for an empty table, using next(). This has been committed as r38943.

@nnposter
Copy link

nnposter commented Jul 4, 2024

The second commit fixes an issue I saw where the SqlServer returned no rows of data. The loop was being entered even though the pos var was already past the length of the data.

The way I understand this, the real issue is that the loop above that, which is tasked with finding the first ROW token, is eating up the DONE token, which otherwise causes the subsequent row-populating loop to terminate. Therefore it seems to me that a cleaner way to rectify the issue would be to bail out from the first loop not just when encountering the ROW token but also on DONE.

This has been committed as r38945. Please test it if possible.

As a side note, having true in expression true and pos < data:len() serves no purpose.

nmap-bot pushed a commit that referenced this pull request Jul 4, 2024
@nmap-bot nmap-bot closed this in a0d24d0 Jul 4, 2024
@nnposter
Copy link

nnposter commented Jul 4, 2024

First commit corrects the return type expectation for a call made to Helper.GetDiscoveredInstances. The actual return is a single value, while the original expected return by the caller was a tuple. I corrected the caller to use only the single value.

This change has been committed as r38948, along with fixing another similar bug. Thank you for contributing to Nmap!

@nnposter nnposter self-assigned this Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants