microcosm-cc / bluemonday

bluemonday: a fast golang HTML sanitizer (inspired by the OWASP Java HTML Sanitizer) to scrub user generated content of XSS
https://github.com/microcosm-cc/bluemonday
BSD 3-Clause "New" or "Revised" License
3.2k stars 175 forks source link

fatal error: concurrent map writes #75

Open Sakibs opened 6 years ago

Sakibs commented 6 years ago

We use bluemonday in our golang service and under high load and traffic we are noticing the error below fatal error: concurrent map writes

fatal error: concurrent map writes

goroutine 239204 [running]:
runtime.throw(0xbea50a, 0x15)
#011/usr/lib/go-1.9/src/runtime/panic.go:605 +0x95 fp=0xc42157a508 sp=0xc42157a4e8 pc=0x42bd85
runtime.mapassign_faststr(0xaf4580, 0xc4201c3380, 0xbdc0da, 0x5, 0xc42025e168)
#011/usr/lib/go-1.9/src/runtime/hashmap_fast.go:861 +0x4da fp=0xc42157a588 sp=0xc42157a508 pc=0x40d41a
git.corp.adobe.com/service/vendor/github.com/microcosm-cc/bluemonday.(*attrPolicyBuilder).OnElements(0xc420321e60, 0xc42157a650, 0x1, 0x1, 0xc420321e60)
#011/go/src/git.corp.adobe.com/service/vendor/github.com/microcosm-cc/bluemonday/policy.go:176 +0x139 fp=0xc42157a620 sp=0xc42157a588 pc=0x9be669
git.corp.adobe.com/service/jobs.sanitizeComment(0xc4203edec0, 0xc, 0x32, 0xc421041e80)
grafana-dee commented 6 years ago

How are you using this?

Policy creation is not thread safe, and you should create the policy at startup (or guaranteed just once if you lazy create it later).

Sanitization is thread safe and once a policy has been created you can use it across multiple goroutines safely assuming you are not modifying the policy.

Sakibs commented 6 years ago

I have it at a package level global variable. I then just use it in a function

package mypkg

var (
    // https://godoc.org/github.com/microcosm-cc/bluemonday#StrictPolicy
    strictPolicy = bluemonday.StrictPolicy()
    // https://godoc.org/github.com/microcosm-cc/bluemonday#UGCPolicy
    ugcPolicy = bluemonday.UGCPolicy()
)

func sanitizeComment(body string) string {
    // allow <span> tag styles
    ugcPolicy.AllowAttrs("style").OnElements("span")  // *** PANICS HERE ***
    result := ugcPolicy.Sanitize(body)

        return result
}
grafana-dee commented 6 years ago

Yup, it is the modifying of the policy that isn't thread safe.

I'll drop an example in here once I'm at work.

luckydonald commented 5 years ago

Huh.