corazawaf / libinjection-go

libinjection is a Golang port of the libinjection(https://github.com/client9/libinjection)
BSD 3-Clause "New" or "Revised" License
37 stars 6 forks source link

fix: inline tag closing check when reading attribute name to prevent deep recursion #17

Closed MirkoDziadzka closed 3 months ago

MirkoDziadzka commented 1 year ago

There is a recursion loop between stateSelfClosingStartTag and stateSelfClosingStartTag which in the worst case consumes 2 stack frames per input character.

We avoid this recursion by manual implementing tail call optimization (TCO) which a go compiler does not do automatically.

Also add a unit test to reproduce the problem in the first place.

This fixes #16

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 85.00% and project coverage change: -0.11 :warning:

Comparison is base (b624886) 90.21% compared to head (2247793) 90.11%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #17 +/- ## ========================================== - Coverage 90.21% 90.11% -0.11% ========================================== Files 8 8 Lines 1523 1527 +4 ========================================== + Hits 1374 1376 +2 - Misses 128 130 +2 Partials 21 21 ``` | [Impacted Files](https://codecov.io/gh/corazawaf/libinjection-go/pull/17?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf) | Coverage Δ | | |---|---|---| | [html5.go](https://codecov.io/gh/corazawaf/libinjection-go/pull/17?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf#diff-aHRtbDUuZ28=) | `87.73% <85.00%> (-0.36%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=corazawaf)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jcchavezs commented 1 year ago

Ping @anuraaga @manojgop @ns-sundar

jcchavezs commented 1 year ago

There is a sister PR here https://github.com/libinjection/libinjection/pull/36

sonarcloud[bot] commented 1 year ago

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

anuraaga commented 1 year ago

I have pushed a commit which I think makes this more idiomatic to Go and readable for future authors, TCO is a feature of compilers, not of code.

@jcchavezs This repo needs to migrate to our standard tooling. Can we create a magefiles repo in the org to not copy-paste?

anuraaga commented 1 year ago

And yeah, IMO the long term refactor seems to be to make sure there is only one top-level processing loop where state transitions happen, rather than happen state transitions and iteration to happen piecemeal within different functions.

jcchavezs commented 1 year ago

Yeah we can create standard magefiles inside corazawaf. At least for lint, test, format, tidy this should do the trick.

On Mon, 27 Mar 2023, 03:21 Anuraag Agrawal, @.***> wrote:

And yeah, IMO the long term refactor seems to be to make sure there is only one top-level processing loop where state transitions happen, rather than happen state transitions and iteration to happen piecemeal within different functions.

— Reply to this email directly, view it on GitHub https://github.com/corazawaf/libinjection-go/pull/17#issuecomment-1484328670, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYAW5DJPGKUCERRO7233W6DTR7ANCNFSM6AAAAAAWHBVQNI . You are receiving this because you were mentioned.Message ID: @.***>

fzipi commented 7 months ago

@anuraaga @jcchavezs what is the state of this change? are we pushing this one and then doing a better change?

fzipi commented 3 months ago

Merging.