From 5464ab37d2ede542f62629edcd3df6fa1a4d46c3 Mon Sep 17 00:00:00 2001 From: Niklas Fasching Date: Sun, 12 Mar 2023 11:08:19 +0100 Subject: [PATCH] Fix race condition surrounding global orgWriter Since writers are normally only used synchronously (i.e. to write one document at a time), we don't guard modifications to their internal state (e.g. temporarily replacing the string.Builder in WriteNodesAsString) against race conditions. The package global `orgWriter` and corresponding use cases of it (`org.String`, `$node.String`) break that pattern - the writer is potentially used from multiple go routines at the same time. This results in race conditions that manifest as error messages like e.g. could not write output: runtime error: invalid memory address or nil pointer dereference. Using unrendered content. Additionally, since we catch panics in `Document.Write`, the corresponding stack trace is lost and dependents of go-org never know what hit them. As using a writer across simultaneously across go routines is not a standard pattern, we'll sync the use of the global `orgWriter` instead of trying to make the actual writer threadsafe; less code noise for the common use case. --- blorg/config.go | 2 +- org/block.go | 8 ++++---- org/document.go | 8 +++++++- org/drawer.go | 4 ++-- org/footnote.go | 2 +- org/headline.go | 2 +- org/html_writer.go | 4 ++-- org/inline.go | 24 ++++++++++++------------ org/keyword.go | 10 +++++----- org/list.go | 6 +++--- org/paragraph.go | 4 ++-- org/table.go | 2 +- 12 files changed, 41 insertions(+), 35 deletions(-) diff --git a/blorg/config.go b/blorg/config.go index c16c211..8aaefa4 100644 --- a/blorg/config.go +++ b/blorg/config.go @@ -105,7 +105,7 @@ func ReadConfig(configFile string) (*Config, error) { if block.Parameters[0] != "html" { continue } - if _, err := config.Template.New(name).Parse(org.String(block.Children)); err != nil { + if _, err := config.Template.New(name).Parse(org.String(block.Children...)); err != nil { return nil, err } } diff --git a/org/block.go b/org/block.go index 2f30a7a..6907eaa 100644 --- a/org/block.go +++ b/org/block.go @@ -177,7 +177,7 @@ func (b Block) ParameterMap() map[string]string { return m } -func (n Example) String() string { return orgWriter.WriteNodesAsString(n) } -func (n Block) String() string { return orgWriter.WriteNodesAsString(n) } -func (n LatexBlock) String() string { return orgWriter.WriteNodesAsString(n) } -func (n Result) String() string { return orgWriter.WriteNodesAsString(n) } +func (n Example) String() string { return String(n) } +func (n Block) String() string { return String(n) } +func (n LatexBlock) String() string { return String(n) } +func (n Result) String() string { return String(n) } diff --git a/org/document.go b/org/document.go index 9da27b9..7357fda 100644 --- a/org/document.go +++ b/org/document.go @@ -20,6 +20,7 @@ import ( "log" "os" "strings" + "sync" ) type Configuration struct { @@ -77,6 +78,7 @@ var lexFns = []lexFn{ } var nilToken = token{"nil", -1, "", nil} +var orgWriterMutex = sync.Mutex{} var orgWriter = NewOrgWriter() // New returns a new Configuration with (hopefully) sane defaults. @@ -95,7 +97,11 @@ func New() *Configuration { } // String returns the pretty printed Org mode string for the given nodes (see OrgWriter). -func String(nodes []Node) string { return orgWriter.WriteNodesAsString(nodes...) } +func String(nodes ...Node) string { + orgWriterMutex.Lock() + defer orgWriterMutex.Unlock() + return orgWriter.WriteNodesAsString(nodes...) +} // Write is called after with an instance of the Writer interface to export a parsed Document into another format. func (d *Document) Write(w Writer) (out string, err error) { diff --git a/org/drawer.go b/org/drawer.go index 5576302..36ef745 100644 --- a/org/drawer.go +++ b/org/drawer.go @@ -93,5 +93,5 @@ func (d *PropertyDrawer) Get(key string) (string, bool) { return "", false } -func (n Drawer) String() string { return orgWriter.WriteNodesAsString(n) } -func (n PropertyDrawer) String() string { return orgWriter.WriteNodesAsString(n) } +func (n Drawer) String() string { return String(n) } +func (n PropertyDrawer) String() string { return String(n) } diff --git a/org/footnote.go b/org/footnote.go index e860421..a96a565 100644 --- a/org/footnote.go +++ b/org/footnote.go @@ -32,4 +32,4 @@ func (d *Document) parseFootnoteDefinition(i int, parentStop stopFn) (int, Node) return consumed, definition } -func (n FootnoteDefinition) String() string { return orgWriter.WriteNodesAsString(n) } +func (n FootnoteDefinition) String() string { return String(n) } diff --git a/org/headline.go b/org/headline.go index c306aec..f9a77c7 100644 --- a/org/headline.go +++ b/org/headline.go @@ -126,4 +126,4 @@ func (parent *Section) add(current *Section) { } } -func (n Headline) String() string { return orgWriter.WriteNodesAsString(n) } +func (n Headline) String() string { return String(n) } diff --git a/org/html_writer.go b/org/html_writer.go index fa458be..ef27089 100644 --- a/org/html_writer.go +++ b/org/html_writer.go @@ -409,14 +409,14 @@ func (w *HTMLWriter) WriteRegularLink(l RegularLink) { if l.Description == nil { w.WriteString(fmt.Sprintf(`%s`, url, url, url)) } else { - description := strings.TrimPrefix(String(l.Description), "file:") + description := strings.TrimPrefix(String(l.Description...), "file:") w.WriteString(fmt.Sprintf(`%s`, url, description, description)) } case "video": if l.Description == nil { w.WriteString(fmt.Sprintf(``, url, url, url)) } else { - description := strings.TrimPrefix(String(l.Description), "file:") + description := strings.TrimPrefix(String(l.Description...), "file:") w.WriteString(fmt.Sprintf(``, url, description, description)) } default: diff --git a/org/inline.go b/org/inline.go index d94a22e..03aea45 100644 --- a/org/inline.go +++ b/org/inline.go @@ -400,7 +400,7 @@ func isValidPostChar(r rune) bool { func isValidBorderChar(r rune) bool { return !unicode.IsSpace(r) } func (l RegularLink) Kind() string { - description := String(l.Description) + description := String(l.Description...) descProtocol, descExt := strings.SplitN(description, ":", 2)[0], path.Ext(description) if ok := descProtocol == "file" || descProtocol == "http" || descProtocol == "https"; ok && imageExtensionRegexp.MatchString(descExt) { return "image" @@ -420,14 +420,14 @@ func (l RegularLink) Kind() string { return "regular" } -func (n Text) String() string { return orgWriter.WriteNodesAsString(n) } -func (n LineBreak) String() string { return orgWriter.WriteNodesAsString(n) } -func (n ExplicitLineBreak) String() string { return orgWriter.WriteNodesAsString(n) } -func (n StatisticToken) String() string { return orgWriter.WriteNodesAsString(n) } -func (n Emphasis) String() string { return orgWriter.WriteNodesAsString(n) } -func (n InlineBlock) String() string { return orgWriter.WriteNodesAsString(n) } -func (n LatexFragment) String() string { return orgWriter.WriteNodesAsString(n) } -func (n FootnoteLink) String() string { return orgWriter.WriteNodesAsString(n) } -func (n RegularLink) String() string { return orgWriter.WriteNodesAsString(n) } -func (n Macro) String() string { return orgWriter.WriteNodesAsString(n) } -func (n Timestamp) String() string { return orgWriter.WriteNodesAsString(n) } +func (n Text) String() string { return String(n) } +func (n LineBreak) String() string { return String(n) } +func (n ExplicitLineBreak) String() string { return String(n) } +func (n StatisticToken) String() string { return String(n) } +func (n Emphasis) String() string { return String(n) } +func (n InlineBlock) String() string { return String(n) } +func (n LatexFragment) String() string { return String(n) } +func (n FootnoteLink) String() string { return String(n) } +func (n RegularLink) String() string { return String(n) } +func (n Macro) String() string { return String(n) } +func (n Timestamp) String() string { return String(n) } diff --git a/org/keyword.go b/org/keyword.go index 1af62fc..b0f49c5 100644 --- a/org/keyword.go +++ b/org/keyword.go @@ -187,8 +187,8 @@ func (d *Document) loadSetupFile(k Keyword) (int, Node) { return 1, k } -func (n Comment) String() string { return orgWriter.WriteNodesAsString(n) } -func (n Keyword) String() string { return orgWriter.WriteNodesAsString(n) } -func (n NodeWithMeta) String() string { return orgWriter.WriteNodesAsString(n) } -func (n NodeWithName) String() string { return orgWriter.WriteNodesAsString(n) } -func (n Include) String() string { return orgWriter.WriteNodesAsString(n) } +func (n Comment) String() string { return String(n) } +func (n Keyword) String() string { return String(n) } +func (n NodeWithMeta) String() string { return String(n) } +func (n NodeWithName) String() string { return String(n) } +func (n Include) String() string { return String(n) } diff --git a/org/list.go b/org/list.go index 9fcf31c..9deabea 100644 --- a/org/list.go +++ b/org/list.go @@ -118,6 +118,6 @@ func (d *Document) parseListItem(l List, i int, parentStop stopFn) (int, Node) { return i - start, ListItem{bullet, status, value, nodes} } -func (n List) String() string { return orgWriter.WriteNodesAsString(n) } -func (n ListItem) String() string { return orgWriter.WriteNodesAsString(n) } -func (n DescriptiveListItem) String() string { return orgWriter.WriteNodesAsString(n) } +func (n List) String() string { return String(n) } +func (n ListItem) String() string { return String(n) } +func (n DescriptiveListItem) String() string { return String(n) } diff --git a/org/paragraph.go b/org/paragraph.go index 2c58eac..1e93d40 100644 --- a/org/paragraph.go +++ b/org/paragraph.go @@ -43,5 +43,5 @@ func (d *Document) parseHorizontalRule(i int, parentStop stopFn) (int, Node) { return 1, HorizontalRule{} } -func (n Paragraph) String() string { return orgWriter.WriteNodesAsString(n) } -func (n HorizontalRule) String() string { return orgWriter.WriteNodesAsString(n) } +func (n Paragraph) String() string { return String(n) } +func (n HorizontalRule) String() string { return String(n) } diff --git a/org/table.go b/org/table.go index 924a64f..57a53e7 100644 --- a/org/table.go +++ b/org/table.go @@ -134,4 +134,4 @@ func isSpecialRow(rawColumns []string) bool { return isAlignRow } -func (n Table) String() string { return orgWriter.WriteNodesAsString(n) } +func (n Table) String() string { return String(n) }