Skip to content

fix #148 #149

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.

Sign up for GitHub

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

Jump to bottom
Closed
parrasajad wants to merge 1 commit into from
Closed

fix #148 #149

parrasajad wants to merge 1 commit into from

Conversation

@parrasajad
Copy link
Contributor

@parrasajad parrasajad commented Jun 11, 2022

No description provided.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 11, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

This was linked to issues Jun 11, 2022
-bulk mode doesn't work with input length less than char-limit #148
Closed
-char-limit is not honoured in non-bulk mode #137
Closed
ehsandeep
ehsandeep requested changes Jun 11, 2022
View reviewed changes
Copy link
Member

@ehsandeep ehsandeep left a comment

Choose a reason for hiding this comment

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

This seems to be fix the issue, however here an edge case causing a crash.

echo test | ./notify -bulk -cl 1

             __  _ ___    
  ___  ___  / /_(_) _/_ __
 / _ \/ _ \/ __/ / _/ // /
/_//_/\___/\__/_/_/ \_, / v1.0.2-dev
                   /___/  

		projectdiscovery.io

Use with caution. You are responsible for your actions
Developers assume no liability and are not responsible for any misuse or damage.
Using default provider config: /Users/geekboy/.config/notify/provider-config.yaml
panic: runtime error: slice bounds out of range [:-2]

goroutine 1 [running]:
github.com/projectdiscovery/notify/internal/runner.bulkSplitter.func1({0x1400032fc60, 0x5, 0x5}, 0x0)
	/Users/geekboy/Github/notify/internal/runner/util.go:36 +0x2e8
bufio.(*Scanner).Scan(0x1400031fe18)
	/opt/homebrew/Cellar/go/1.18.2/libexec/src/bufio/scan.go:147 +0xac
github.com/projectdiscovery/notify/internal/runner.(*Runner).Run(0x14000323020)
	/Users/geekboy/Github/notify/internal/runner/runner.go:112 +0x5b8
main.main()
	/Users/geekboy/Github/notify/cmd/notify/notify.go:42 +0x184

@ehsandeep ehsandeep mentioned this pull request Jun 11, 2022
Merged
@justinsteven
Copy link
Contributor

justinsteven commented Jun 11, 2022

stdin is still broken:

% id | ./notify

             __  _ ___
  ___  ___  / /_(_) _/_ __
 / _ \/ _ \/ __/ / _/ // /
/_//_/\___/\__/_/_/ \_, / v1.0.2-dev
                   /___/

                projectdiscovery.io

Use with caution. You are responsible for your actions
Developers assume no liability and are not responsible for any misuse or damage.
Using default provider config: /home/justin/.config/notify/provider-config.yaml

It's also causing files, when sent in -bulk mode, to have a trailing newline:

% cat AAAA
AAAA
% ./notify -bulk -i AAAA

             __  _ ___
  ___  ___  / /_(_) _/_ __
 / _ \/ _ \/ __/ / _/ // /
/_//_/\___/\__/_/_/ \_, / v1.0.2-dev
                   /___/

                projectdiscovery.io

Use with caution. You are responsible for your actions
Developers assume no liability and are not responsible for any misuse or damage.
Using default provider config: /home/justin/.config/notify/provider-config.yaml
AAAA

%

Finally the change here doesn't seem relevant to the changes in #146 which introduced the problems. Returning 0, nil, nil is correct as it's a special signal to the Scanner that more data is needed to produce a chunk. Perhaps us cranking up the buffer size in #146 means that we won't get a case where we need more data from the Scanner, but this means that the SplitFunc is now not re-usable elsewhere, and we shouldn't be cranking up the buffer size anyway, in doing so we're losing the benefit of using a Scanner which is to be able to stream data in in a memory-efficient way.

I'm sorry for being critical but I don't think this should be merged, and I think #146 needs to be reworked.

@justinsteven
Copy link
Contributor

justinsteven commented Jun 11, 2022

Aha

  • stdin fails on Linux for any input size
  • stdin fails on macOS for small inputs
  • stdin fails on macOS for large inputs (confirmed with a 9000 byte input)

That might be contributing to the confusion around stdin being broken

#146 stat's the input to get its size so as to set the buffer size. On Linux, and on macOS with large inputs, stdin has a reported size of 0.

I will look to rework #146 when I get a chance

@justinsteven justinsteven mentioned this pull request Jun 12, 2022
Merged
@Mzack9999 Mzack9999 added the Status: On Hold Similar to blocked, but is assigned to someone label Jun 12, 2022
@Mzack9999
Copy link
Member

Mzack9999 commented Jun 12, 2022 •
edited
Loading

Thanks @justinsteven for having a look at this. I'm marking the PR on hold until #150 gets reviewed superseded by #150

@Mzack9999 Mzack9999 added the Status: Abandoned This issue is no longer important to the requestor and no one else has shown an interest in it. label Jun 13, 2022
@ehsandeep ehsandeep closed this Jun 13, 2022
@ehsandeep ehsandeep deleted the bulk-mode-fix branch June 13, 2022 11:50
@ehsandeep ehsandeep removed the Status: On Hold Similar to blocked, but is assigned to someone label Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ehsandeep ehsandeep ehsandeep requested changes

@Mzack9999 Mzack9999 Awaiting requested review from Mzack9999

Assignees

No one assigned

Labels

Status: Abandoned This issue is no longer important to the requestor and no one else has shown an interest in it.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

-bulk mode doesn't work with input length less than char-limit -char-limit is not honoured in non-bulk mode

5 participants

@parrasajad @justinsteven @Mzack9999 @ehsandeep