Fix event formatting with missing params

CHANGELOG.asciidoc

@@ -128,6 +128,7 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di
 - Fix the registry file. It was not correctly storing event log names, and
   upon restart it would begin reading at the start of each event log. {issue}5813[5813]
 - Fix config validation to allow `event_logs.processors`. [pull]6217[6217]
+- Fixed a crash under Windows 2003 and XP when an event had less insert strings than required by its format string. {pull}6247[6247]
 
 ==== Added
 

winlogbeat/eventlog/eventlogging.go

@@ -64,6 +64,7 @@ type eventLogging struct {
 	handle    win.Handle         // Handle to the event log.
 	readBuf   []byte             // Buffer for reading in events.
 	formatBuf []byte             // Buffer for formatting messages.
+	insertBuf win.StringInserts  // Buffer for parsing insert strings.
 	handles   *messageFilesCache // Cached mapping of source name to event message file handles.
 	logPrefix string             // Prefix to add to all log entries.
 
@@ -149,7 +150,7 @@ func (l *eventLogging) Read() ([]Record, error) {
 
 	l.readBuf = l.readBuf[0:numBytesRead]
 	events, _, err := win.RenderEvents(
-		l.readBuf[:numBytesRead], 0, l.formatBuf, l.handles.get)
+		l.readBuf[:numBytesRead], 0, l.formatBuf, &l.insertBuf, l.handles.get)
 	if err != nil {
 		return nil, err
 	}

winlogbeat/eventlog/eventlogging_test.go

@@ -475,6 +475,49 @@ func TestReadWhileCleared(t *testing.T) {
 	}
 }
 
+// Test event messages that include less parameters than required for message
+// formating (caused a crash in previous versions)
+func TestReadMissingParameters(t *testing.T) {
+	configureLogp()
+	log, err := initLog(providerName, sourceName, servicesMsgFile)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() {
+		err := uninstallLog(providerName, sourceName, log)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}()
+
+	var eventID uint32 = 1073748860
+	// Missing parameters will be substituted by "(null)"
+	template := "The %s service entered the (null) state."
+	msgs := []string{"Windows Update"}
+	err = log.Report(elog.Info, eventID, msgs)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Read messages:
+	eventlog, teardown := setupEventLogging(t, 0, map[string]interface{}{"name": providerName})
+	defer teardown()
+
+	records, err := eventlog.Read()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Verify the message contents:
+	assert.Len(t, records, 1)
+	if len(records) != 1 {
+		t.FailNow()
+	}
+	assert.Equal(t, eventID&0xFFFF, records[0].EventIdentifier.ID)
+	assert.Equal(t, fmt.Sprintf(template, msgs[0]),
+		strings.TrimRight(records[0].Message, "\r\n"))
+}
+
 func newTestEventLogging(t *testing.T, options map[string]interface{}) EventLog {
 	return newTestEventLog(t, newEventLogging, options)
 }

winlogbeat/sys/eventlogging/eventlogging_windows.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"encoding/binary"
 	"fmt"
-	"reflect"
 	"strings"
 	"syscall"
 	"time"
@@ -100,6 +99,7 @@ func RenderEvents(
 	eventsRaw []byte,
 	lang uint32,
 	buffer []byte,
+	insertStrings *StringInserts,
 	pubHandleProvider func(string) sys.MessageFiles,
 ) ([]sys.Event, int, error) {
 	var events []sys.Event
@@ -139,21 +139,25 @@ func RenderEvents(
 			event.User = *sid
 		}
 
+		if record.numStrings > MaxInsertStrings {
+			logp.Warn("Record contains %d strings, more than the limit %d. Excess will be ignored.",
+				record.numStrings, MaxInsertStrings)
+			record.numStrings = MaxInsertStrings
+		}
 		// Parse the UTF-16 message insert strings.
-		stringInserts, stringInsertPtrs, err := parseInsertStrings(record, recordBuf)
-		if err != nil {
+		if err = insertStrings.Parse(record, recordBuf); err != nil {
 			event.RenderErr = err.Error()
 			events = append(events, event)
 			continue
 		}
 
-		for _, s := range stringInserts {
+		for _, s := range insertStrings.Strings() {
 			event.EventData.Pairs = append(event.EventData.Pairs, sys.KeyValue{Value: s})
 		}
 
 		// Format the parametrized message using the insert strings.
 		event.Message, err = formatMessage(record.sourceName,
-			record.eventID, lang, stringInsertPtrs, buffer, pubHandleProvider)
+			record.eventID, lang, insertStrings.Pointer(), buffer, pubHandleProvider)
 		if err != nil {
 			event.RenderErr = err.Error()
 			if errno, ok := err.(syscall.Errno); ok {
@@ -180,15 +184,10 @@ func formatMessage(
 	sourceName string,
 	eventID uint32,
 	lang uint32,
-	stringInserts []uintptr,
+	stringInserts uintptr,
 	buffer []byte,
 	pubHandleProvider func(string) sys.MessageFiles,
 ) (string, error) {
-	var addr uintptr
-	if len(stringInserts) > 0 {
-		addr = reflect.ValueOf(&stringInserts[0]).Pointer()
-	}
-
 	messageFiles := pubHandleProvider(sourceName)
 
 	var lastErr error
@@ -208,7 +207,7 @@ func formatMessage(
 			lang,
 			&buffer[0],            // Max size allowed is 64k bytes.
 			uint32(len(buffer)/2), // Size of buffer in TCHARS
-			addr)
+			stringInserts)
 		// bufferUsed = numChars * sizeof(TCHAR) + sizeof(null-terminator)
 		bufferUsed := int(numChars*2 + 2)
 		if err == syscall.ERROR_INSUFFICIENT_BUFFER {
@@ -391,38 +390,6 @@ func parseEventLogRecord(buffer []byte) (eventLogRecord, error) {
 	return record, nil
 }
 
-// parseInsertStrings parses the insert strings from buffer which should contain
-// an eventLogRecord. It returns an array of strings (data is copied and
-// converted to UTF-8) and an array of pointers to the null-terminated UTF-16
-// strings within buffer.
-func parseInsertStrings(record eventLogRecord, buffer []byte) ([]string, []uintptr, error) {
-	if record.numStrings < 1 {
-		return nil, nil, nil
-	}
-
-	inserts := make([]string, record.numStrings)
-	insertPtrs := make([]uintptr, record.numStrings)
-	offset := int(record.stringOffset)
-	bufferPtr := reflect.ValueOf(&buffer[0]).Pointer()
-
-	for i := 0; i < int(record.numStrings); i++ {
-		if offset > len(buffer) {
-			return nil, nil, fmt.Errorf("Failed reading string number %d, "+
-				"offset=%d, len(buffer)=%d, record=%+v", i+1, offset,
-				len(buffer), record)
-		}
-		insertStr, length, err := sys.UTF16BytesToString(buffer[offset:])
-		if err != nil {
-			return nil, nil, err
-		}
-		inserts[i] = insertStr
-		insertPtrs[i] = bufferPtr + uintptr(offset)
-		offset += length
-	}
-
-	return inserts, insertPtrs, nil
-}
-
 func parseSID(record eventLogRecord, buffer []byte) (*sys.SID, error) {
 	if record.userSidLength == 0 {
 		return nil, nil

winlogbeat/sys/eventlogging/stringinserts_windows.go

@@ -0,0 +1,86 @@
+package eventlogging
+
+import (
+	"fmt"
+	"reflect"
+	"unsafe"
+
+	"github.com/elastic/beats/winlogbeat/sys"
+)
+
+const (
+	// MaxInsertStrings is the maximum number of strings that can be formatted by
+	// FormatMessage API.
+	MaxInsertStrings = 99
+)
+
+var (
+	nullPlaceholder    = []byte{'(', 0, 'n', 0, 'u', 0, 'l', 0, 'l', 0, ')', 0, 0, 0}
+	nullPlaceholderPtr = uintptr(unsafe.Pointer(&nullPlaceholder[0]))
+)
+
+// StringInserts stores the string inserts for an event, as arrays of string
+// and pointer to UTF-16 zero-terminated string suitable to be passed to
+// the Windows API. The array of pointers has enough entries to ensure that
+// a call to FormatMessage will never crash.
+type StringInserts struct {
+	pointers [MaxInsertStrings]uintptr
+	inserts  []string
+	address  uintptr
+}
+
+// Parse parses the insert strings from buffer which should contain
+// an eventLogRecord.
+func (b *StringInserts) Parse(record eventLogRecord, buffer []byte) error {
+	if b.inserts == nil { // initialise struct
+		b.inserts = make([]string, 0, MaxInsertStrings)
+		b.address = reflect.ValueOf(&b.pointers[0]).Pointer()
+	}
+	b.clear()
+
+	n := int(record.numStrings)
+	if n > MaxInsertStrings {
+		return fmt.Errorf("number of insert strings in the record (%d) is larger than the limit (%d)", n, MaxInsertStrings)
+	}
+
+	b.inserts = b.inserts[:n]
+	if n == 0 {
+		return nil
+	}
+	offset := int(record.stringOffset)
+	bufferPtr := reflect.ValueOf(&buffer[0]).Pointer()
+
+	for i := 0; i < n; i++ {
+		if offset > len(buffer) {
+			return fmt.Errorf("Failed reading string number %d, "+
+				"offset=%d, len(buffer)=%d, record=%+v", i+1, offset,
+				len(buffer), record)
+		}
+		insertStr, length, err := sys.UTF16BytesToString(buffer[offset:])
+		if err != nil {
+			return err
+		}
+		b.inserts[i] = insertStr
+		b.pointers[i] = bufferPtr + uintptr(offset)
+		offset += length
+	}
+
+	return nil
+}
+
+// Strings returns the array of strings representing the insert strings.
+func (b *StringInserts) Strings() []string {
+	return b.inserts
+}
+
+// Pointer returns a pointer to an array of UTF-16 strings suitable to be
+// passed to the FormatMessage API.
+func (b *StringInserts) Pointer() uintptr {
+	return b.address
+}
+
+func (b *StringInserts) clear() {
+	for i := 0; i < MaxInsertStrings && b.pointers[i] != nullPlaceholderPtr; i++ {
+		b.pointers[i] = nullPlaceholderPtr
+	}
+}