From 02000c3c26e32267483dd1a5e976c70afd1d90db Mon Sep 17 00:00:00 2001 From: Marc Guasch Date: Fri, 19 Mar 2021 16:06:14 +0100 Subject: [PATCH] [Winlogbeat] Remove redundant code and move to common package (#24560) * Move safe reader to libbeat/common/encoding/xml package * Remove redundant code and move to winlogbeat/sys package * Add changelog entry * Add safe_reader.go header * Apply suggestions from code review Co-authored-by: Andrew Kroh * Update CHANGELOG.next.asciidoc Co-authored-by: Andrew Kroh --- .../common/encoding/xml/safe_reader.go | 14 +-- winlogbeat/sys/buffer.go | 21 ++++ winlogbeat/sys/bufferpool.go | 48 ++++++++ winlogbeat/sys/event.go | 4 +- winlogbeat/sys/strings.go | 56 ++------- winlogbeat/sys/strings_test.go | 49 +------- winlogbeat/sys/wineventlog/bookmark.go | 10 +- winlogbeat/sys/wineventlog/bufferpool.go | 113 ------------------ winlogbeat/sys/wineventlog/format_message.go | 10 +- winlogbeat/sys/wineventlog/renderer.go | 24 ++-- winlogbeat/sys/wineventlog/syscall_windows.go | 4 +- .../sys/wineventlog/wineventlog_windows.go | 2 +- 12 files changed, 119 insertions(+), 236 deletions(-) rename winlogbeat/sys/xmlreader.go => libbeat/common/encoding/xml/safe_reader.go (85%) create mode 100644 winlogbeat/sys/bufferpool.go delete mode 100644 winlogbeat/sys/wineventlog/bufferpool.go diff --git a/winlogbeat/sys/xmlreader.go b/libbeat/common/encoding/xml/safe_reader.go similarity index 85% rename from winlogbeat/sys/xmlreader.go rename to libbeat/common/encoding/xml/safe_reader.go index ba51c90dcd5..9c7c95c73c6 100644 --- a/winlogbeat/sys/xmlreader.go +++ b/libbeat/common/encoding/xml/safe_reader.go @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -package sys +package xml import ( "bytes" @@ -25,18 +25,18 @@ import ( "unicode/utf8" ) -// The type xmlSafeReader escapes UTF control characters in the io.Reader +// A SafeReader escapes UTF control characters in the io.Reader // it wraps, so that it can be fed to Go's xml parser. // Characters for which `unicode.IsControl` returns true will be output as // an hexadecimal unicode escape sequence "\\uNNNN". -type xmlSafeReader struct { +type SafeReader struct { inner io.Reader backing [256]byte buf []byte code []byte } -var _ io.Reader = (*xmlSafeReader)(nil) +var _ io.Reader = (*SafeReader)(nil) func output(n int) (int, error) { if n == 0 { @@ -46,7 +46,7 @@ func output(n int) (int, error) { } // Read implements the io.Reader interface. -func (r *xmlSafeReader) Read(d []byte) (n int, err error) { +func (r *SafeReader) Read(d []byte) (n int, err error) { if len(r.code) > 0 { n = copy(d, r.code) r.code = r.code[n:] @@ -73,6 +73,6 @@ func (r *xmlSafeReader) Read(d []byte) (n int, err error) { return output(n) } -func newXMLSafeReader(rawXML []byte) io.Reader { - return &xmlSafeReader{inner: bytes.NewReader(rawXML)} +func NewSafeReader(rawXML []byte) *SafeReader { + return &SafeReader{inner: bytes.NewReader(rawXML)} } diff --git a/winlogbeat/sys/buffer.go b/winlogbeat/sys/buffer.go index 97299a20913..d7721b5adc5 100644 --- a/winlogbeat/sys/buffer.go +++ b/winlogbeat/sys/buffer.go @@ -61,3 +61,24 @@ func (b *ByteBuffer) Bytes() []byte { func (b *ByteBuffer) Len() int { return b.offset } + +// PtrAt returns a pointer to the given offset of the buffer. +func (b *ByteBuffer) PtrAt(offset int) *byte { + if offset > b.offset-1 { + return nil + } + return &b.buf[offset] +} + +// Reserve reserves n bytes by increasing the buffer's length. It may allocate +// a new underlying buffer discarding any existing contents. +func (b *ByteBuffer) Reserve(n int) { + b.offset = n + + if n > cap(b.buf) { + // Allocate new larger buffer with len=n. + b.buf = make([]byte, n) + } else { + b.buf = b.buf[:n] + } +} diff --git a/winlogbeat/sys/bufferpool.go b/winlogbeat/sys/bufferpool.go new file mode 100644 index 00000000000..ed5bbebfbc6 --- /dev/null +++ b/winlogbeat/sys/bufferpool.go @@ -0,0 +1,48 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package sys + +import ( + "sync" +) + +// bufferPool contains a pool of PooledByteBuffer objects. +var bufferPool = sync.Pool{ + New: func() interface{} { return &PooledByteBuffer{ByteBuffer: NewByteBuffer(1024)} }, +} + +// PooledByteBuffer is an expandable buffer backed by a byte slice. +type PooledByteBuffer struct { + *ByteBuffer +} + +// NewPooledByteBuffer return a PooledByteBuffer from the pool. The returned value must +// be released with Free(). +func NewPooledByteBuffer() *PooledByteBuffer { + b := bufferPool.Get().(*PooledByteBuffer) + b.Reset() + return b +} + +// Free returns the PooledByteBuffer to the pool. +func (b *PooledByteBuffer) Free() { + if b == nil { + return + } + bufferPool.Put(b) +} diff --git a/winlogbeat/sys/event.go b/winlogbeat/sys/event.go index b6674d41f40..6294d75f1a8 100644 --- a/winlogbeat/sys/event.go +++ b/winlogbeat/sys/event.go @@ -22,12 +22,14 @@ import ( "fmt" "strconv" "time" + + libxml "github.com/elastic/beats/v7/libbeat/common/encoding/xml" ) // UnmarshalEventXML unmarshals the given XML into a new Event. func UnmarshalEventXML(rawXML []byte) (Event, error) { var event Event - decoder := xml.NewDecoder(newXMLSafeReader(rawXML)) + decoder := xml.NewDecoder(libxml.NewSafeReader(rawXML)) err := decoder.Decode(&event) return event, err } diff --git a/winlogbeat/sys/strings.go b/winlogbeat/sys/strings.go index 12d91fac31d..d542277f17b 100644 --- a/winlogbeat/sys/strings.go +++ b/winlogbeat/sys/strings.go @@ -18,57 +18,23 @@ package sys import ( - "errors" - "fmt" "strings" - "unicode/utf16" -) - -var ErrBufferTooSmall = errors.New("buffer too small") - -// UTF16BytesToString returns a string that is decoded from the UTF-16 bytes. -// The byte slice must be of even length otherwise an error will be returned. -// The integer returned is the offset to the start of the next string with -// buffer if it exists, otherwise -1 is returned. -func UTF16BytesToString(b []byte) (string, int, error) { - if len(b)%2 != 0 { - return "", 0, fmt.Errorf("Slice must have an even length (length=%d)", len(b)) - } - offset := -1 - - // Find the null terminator if it exists and re-slice the b. - if nullIndex := indexNullTerminator(b); nullIndex > -1 { - if len(b) > nullIndex+2 { - offset = nullIndex + 2 - } - - b = b[:nullIndex] - } - - s := make([]uint16, len(b)/2) - for i := range s { - s[i] = uint16(b[i*2]) + uint16(b[(i*2)+1])<<8 - } - - return string(utf16.Decode(s)), offset, nil -} + "github.com/elastic/beats/v7/libbeat/common" +) -// indexNullTerminator returns the index of a null terminator within a buffer -// containing UTF-16 encoded data. If the null terminator is not found -1 is -// returned. -func indexNullTerminator(b []byte) int { - if len(b) < 2 { - return -1 - } +// UTF16BytesToString converts the given UTF-16 bytes to a string. +func UTF16BytesToString(b []byte) (string, error) { + // Use space from the ByteBuffer pool as working memory for the conversion. + bb := NewPooledByteBuffer() + defer bb.Free() - for i := 0; i < len(b); i += 2 { - if b[i] == 0 && b[i+1] == 0 { - return i - } + if err := common.UTF16ToUTF8Bytes(b, bb); err != nil { + return "", err } - return -1 + // This copies the UTF-8 bytes to create a string. + return string(bb.Bytes()), nil } // RemoveWindowsLineEndings replaces carriage return line feed (CRLF) with diff --git a/winlogbeat/sys/strings_test.go b/winlogbeat/sys/strings_test.go index 358e61ed6b2..0771b7b3cff 100644 --- a/winlogbeat/sys/strings_test.go +++ b/winlogbeat/sys/strings_test.go @@ -18,7 +18,6 @@ package sys import ( - "bytes" "testing" "github.com/stretchr/testify/assert" @@ -30,59 +29,13 @@ func TestUTF16BytesToString(t *testing.T) { input := "abc白鵬翔\u145A6" utf16Bytes := common.StringToUTF16Bytes(input) - output, _, err := UTF16BytesToString(utf16Bytes) + output, err := UTF16BytesToString(utf16Bytes) if err != nil { t.Fatal(err) } assert.Equal(t, input, output) } -func TestUTF16BytesToStringOffset(t *testing.T) { - in := bytes.Join([][]byte{common.StringToUTF16Bytes("one"), common.StringToUTF16Bytes("two"), common.StringToUTF16Bytes("three")}, []byte{0, 0}) - - output, offset, err := UTF16BytesToString(in) - if err != nil { - t.Fatal(err) - } - assert.Equal(t, "one", output) - assert.Equal(t, 8, offset) - - in = in[offset:] - output, offset, err = UTF16BytesToString(in) - if err != nil { - t.Fatal(err) - } - assert.Equal(t, "two", output) - assert.Equal(t, 8, offset) - - in = in[offset:] - output, offset, err = UTF16BytesToString(in) - if err != nil { - t.Fatal(err) - } - assert.Equal(t, "three", output) - assert.Equal(t, -1, offset) -} - -func TestUTF16BytesToStringOffsetWithEmptyString(t *testing.T) { - in := bytes.Join([][]byte{common.StringToUTF16Bytes(""), common.StringToUTF16Bytes("two")}, []byte{0, 0}) - - output, offset, err := UTF16BytesToString(in) - if err != nil { - t.Fatal(err) - } - assert.Equal(t, "", output) - assert.Equal(t, 2, offset) - - in = in[offset:] - output, offset, err = UTF16BytesToString(in) - if err != nil { - t.Fatal(err) - } - assert.Equal(t, "two", output) - assert.Equal(t, -1, offset) -} - func BenchmarkUTF16BytesToString(b *testing.B) { utf16Bytes := common.StringToUTF16Bytes("A logon was attempted using explicit credentials.") diff --git a/winlogbeat/sys/wineventlog/bookmark.go b/winlogbeat/sys/wineventlog/bookmark.go index fa806aa2c34..db9d3d14452 100644 --- a/winlogbeat/sys/wineventlog/bookmark.go +++ b/winlogbeat/sys/wineventlog/bookmark.go @@ -24,6 +24,8 @@ import ( "github.com/pkg/errors" "golang.org/x/sys/windows" + + "github.com/elastic/beats/v7/winlogbeat/sys" ) // Bookmark is a handle to an event log bookmark. @@ -43,16 +45,16 @@ func (b Bookmark) XML() (string, error) { return "", errors.Wrap(err, "failed to determine necessary buffer size for EvtRender") } - bb := newByteBuffer() + bb := sys.NewPooledByteBuffer() bb.Reserve(int(bufferUsed * 2)) - defer bb.free() + defer bb.Free() - err = _EvtRender(NilHandle, EvtHandle(b), EvtRenderBookmark, uint32(len(bb.buf)), &bb.buf[0], &bufferUsed, nil) + err = _EvtRender(NilHandle, EvtHandle(b), EvtRenderBookmark, uint32(bb.Len()), bb.PtrAt(0), &bufferUsed, nil) if err != nil { return "", errors.Wrap(err, "failed to render bookmark XML") } - return UTF16BytesToString(bb.buf) + return sys.UTF16BytesToString(bb.Bytes()) } // NewBookmarkFromEvent returns a Bookmark pointing to the given event record. diff --git a/winlogbeat/sys/wineventlog/bufferpool.go b/winlogbeat/sys/wineventlog/bufferpool.go deleted file mode 100644 index 90bdf825de1..00000000000 --- a/winlogbeat/sys/wineventlog/bufferpool.go +++ /dev/null @@ -1,113 +0,0 @@ -// Licensed to Elasticsearch B.V. under one or more contributor -// license agreements. See the NOTICE file distributed with -// this work for additional information regarding copyright -// ownership. Elasticsearch B.V. licenses this file to you under -// the Apache License, Version 2.0 (the "License"); you may -// not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -package wineventlog - -import ( - "sync" - - "github.com/elastic/beats/v7/libbeat/common" -) - -// bufferPool contains a pool of byteBuffer objects. -var bufferPool = sync.Pool{ - New: func() interface{} { return &byteBuffer{buf: make([]byte, 1024)} }, -} - -// byteBuffer is an expandable buffer backed by a byte slice. -type byteBuffer struct { - buf []byte - offset int -} - -// newByteBuffer return a byteBuffer from the pool. The returned value must -// be released with free(). -func newByteBuffer() *byteBuffer { - b := bufferPool.Get().(*byteBuffer) - b.Reset() - return b -} - -// free returns the byteBuffer to the pool. -func (b *byteBuffer) free() { - if b == nil { - return - } - bufferPool.Put(b) -} - -// Write appends the contents of p to the buffer, growing the buffer as needed. -// The return value is the length of p; err is always nil. This implements -// io.Writer. -func (b *byteBuffer) Write(p []byte) (int, error) { - if len(b.buf) < b.offset+len(p) { - // Create a buffer larger than needed so we don't spend lots of time - // allocating and copying. - spaceNeeded := len(b.buf) - b.offset + len(p) - largerBuf := make([]byte, 2*len(b.buf)+spaceNeeded) - copy(largerBuf, b.buf[:b.offset]) - b.buf = largerBuf - } - n := copy(b.buf[b.offset:], p) - b.offset += n - return n, nil -} - -// Reset resets the buffer to be empty. It retains the same underlying storage -// capacity. -func (b *byteBuffer) Reset() { - b.offset = 0 - b.buf = b.buf[:cap(b.buf)] -} - -// Bytes returns a slice of length b.Len() holding the bytes that have been -// written to the buffer. -func (b *byteBuffer) Bytes() []byte { - return b.buf[:b.offset] -} - -// Len returns the number of bytes that have been written to the buffer. -func (b *byteBuffer) Len() int { - return b.offset -} - -// Reserve reserves n bytes by increasing the buffer's length. It may allocate -// a new underlying buffer discarding any existing contents. -func (b *byteBuffer) Reserve(n int) { - b.offset = n - - if n > cap(b.buf) { - // Allocate new larger buffer with len=n. - b.buf = make([]byte, n) - } else { - b.buf = b.buf[:n] - } -} - -// UTF16BytesToString converts the given UTF-16 bytes to a string. -func UTF16BytesToString(b []byte) (string, error) { - // Use space from the byteBuffer pool as working memory for the conversion. - bb := newByteBuffer() - defer bb.free() - - if err := common.UTF16ToUTF8Bytes(b, bb); err != nil { - return "", err - } - - // This copies the UTF-8 bytes to create a string. - return string(bb.Bytes()), nil -} diff --git a/winlogbeat/sys/wineventlog/format_message.go b/winlogbeat/sys/wineventlog/format_message.go index e8befcdeae3..29182d6f6a0 100644 --- a/winlogbeat/sys/wineventlog/format_message.go +++ b/winlogbeat/sys/wineventlog/format_message.go @@ -24,6 +24,8 @@ import ( "github.com/pkg/errors" "golang.org/x/sys/windows" + + "github.com/elastic/beats/v7/winlogbeat/sys" ) // getMessageStringFromHandle returns the message for the given eventHandle. @@ -82,11 +84,11 @@ func evtFormatMessage(metadataHandle EvtHandle, eventHandle EvtHandle, messageID } // Get a buffer from the pool and adjust its length. - bb := newByteBuffer() - defer bb.free() + bb := sys.NewPooledByteBuffer() + defer bb.Free() bb.Reserve(int(bufferUsed * 2)) - err = _EvtFormatMessage(metadataHandle, eventHandle, messageID, valuesCount, valuesPtr, messageFlag, uint32(len(bb.buf)/2), &bb.buf[0], &bufferUsed) + err = _EvtFormatMessage(metadataHandle, eventHandle, messageID, valuesCount, valuesPtr, messageFlag, uint32(bb.Len()/2), bb.PtrAt(0), &bufferUsed) if err != nil { switch err { // Ignore some errors so it can tolerate missing or mismatched parameter values. @@ -98,5 +100,5 @@ func evtFormatMessage(metadataHandle EvtHandle, eventHandle EvtHandle, messageID } } - return UTF16BytesToString(bb.buf) + return sys.UTF16BytesToString(bb.Bytes()) } diff --git a/winlogbeat/sys/wineventlog/renderer.go b/winlogbeat/sys/wineventlog/renderer.go index 4a6fcc2fef1..2d7c4e47b53 100644 --- a/winlogbeat/sys/wineventlog/renderer.go +++ b/winlogbeat/sys/wineventlog/renderer.go @@ -180,14 +180,14 @@ func (r *Renderer) renderSystem(handle EvtHandle, event *sys.Event) error { if err != nil { return errors.Wrap(err, "failed to get system values") } - defer bb.free() + defer bb.Free() for i := 0; i < int(propertyCount); i++ { property := EvtSystemPropertyID(i) offset := i * int(sizeofEvtVariant) - evtVar := (*EvtVariant)(unsafe.Pointer(&bb.buf[offset])) + evtVar := (*EvtVariant)(unsafe.Pointer(bb.PtrAt(offset))) - data, err := evtVar.Data(bb.buf) + data, err := evtVar.Data(bb.Bytes()) if err != nil || data == nil { continue } @@ -247,7 +247,7 @@ func (r *Renderer) renderUser(handle EvtHandle, event *sys.Event) (values []inte if err != nil { return nil, 0, errors.Wrap(err, "failed to get user values") } - defer bb.free() + defer bb.Free() if propertyCount == 0 { return nil, 0, nil @@ -261,10 +261,10 @@ func (r *Renderer) renderUser(handle EvtHandle, event *sys.Event) (values []inte values = make([]interface{}, propertyCount) for i := 0; i < propertyCount; i++ { offset := i * int(sizeofEvtVariant) - evtVar := (*EvtVariant)(unsafe.Pointer(&bb.buf[offset])) + evtVar := (*EvtVariant)(unsafe.Pointer(bb.PtrAt(offset))) binary.Write(argumentHash, binary.LittleEndian, uint32(evtVar.Type)) - values[i], err = evtVar.Data(bb.buf) + values[i], err = evtVar.Data(bb.Bytes()) if err != nil { r.log.Warnw("Failed to read event/user data value. Using nil.", "provider", event.Provider.Name, @@ -281,7 +281,7 @@ func (r *Renderer) renderUser(handle EvtHandle, event *sys.Event) (values []inte // render uses EvtRender to event data. The caller must free() the returned when // done accessing the bytes. -func (r *Renderer) render(context EvtHandle, eventHandle EvtHandle) (*byteBuffer, int, error) { +func (r *Renderer) render(context EvtHandle, eventHandle EvtHandle) (*sys.PooledByteBuffer, int, error) { var bufferUsed, propertyCount uint32 err := _EvtRender(context, eventHandle, EvtRenderEventValues, 0, nil, &bufferUsed, &propertyCount) @@ -293,12 +293,12 @@ func (r *Renderer) render(context EvtHandle, eventHandle EvtHandle) (*byteBuffer return nil, 0, nil } - bb := newByteBuffer() + bb := sys.NewPooledByteBuffer() bb.Reserve(int(bufferUsed)) - err = _EvtRender(context, eventHandle, EvtRenderEventValues, uint32(len(bb.buf)), &bb.buf[0], &bufferUsed, &propertyCount) + err = _EvtRender(context, eventHandle, EvtRenderEventValues, uint32(bb.Len()), bb.PtrAt(0), &bufferUsed, &propertyCount) if err != nil { - bb.free() + bb.Free() return nil, 0, errors.Wrap(err, "failed in EvtRender") } @@ -384,8 +384,8 @@ func (r *Renderer) formatMessage(publisherMeta *publisherMetadataStore, // formatMessageFromTemplate creates the message by executing the stored Go // text/template with the event/user data values. func (r *Renderer) formatMessageFromTemplate(msgTmpl *template.Template, values []interface{}) (string, error) { - bb := newByteBuffer() - defer bb.free() + bb := sys.NewPooledByteBuffer() + defer bb.Free() if err := msgTmpl.Execute(bb, values); err != nil { return "", errors.Wrapf(err, "failed to execute template with data=%#v template=%v", values, msgTmpl.Root.String()) diff --git a/winlogbeat/sys/wineventlog/syscall_windows.go b/winlogbeat/sys/wineventlog/syscall_windows.go index 8180ae47521..d64c1a804cf 100644 --- a/winlogbeat/sys/wineventlog/syscall_windows.go +++ b/winlogbeat/sys/wineventlog/syscall_windows.go @@ -25,6 +25,8 @@ import ( "github.com/pkg/errors" "golang.org/x/sys/windows" + + "github.com/elastic/beats/v7/winlogbeat/sys" ) // EvtHandle is a handle to the event log. @@ -439,7 +441,7 @@ func (v EvtVariant) Data(buf []byte) (interface{}, error) { case EvtVarTypeString: addr := unsafe.Pointer(&buf[0]) offset := v.ValueAsUintPtr() - uintptr(addr) - s, err := UTF16BytesToString(buf[offset:]) + s, err := sys.UTF16BytesToString(buf[offset:]) return s, err case EvtVarTypeSByte: return int8(v.ValueAsUint8()), nil diff --git a/winlogbeat/sys/wineventlog/wineventlog_windows.go b/winlogbeat/sys/wineventlog/wineventlog_windows.go index 3b282fd41d7..e0f36680320 100644 --- a/winlogbeat/sys/wineventlog/wineventlog_windows.go +++ b/winlogbeat/sys/wineventlog/wineventlog_windows.go @@ -471,7 +471,7 @@ func readString(buffer []byte, reader io.Reader) (string, error) { } return "", err } - str, _, err := sys.UTF16BytesToString(buffer[offset:]) + str, err := sys.UTF16BytesToString(buffer[offset:]) return str, err }