Fix null-terminated string handling in DAT files

internal/geoscape/base_dat.go

@@ -6,6 +6,7 @@ import (
 	"encoding/binary"
 
 	"github.com/go-restruct/restruct"
+	"github.com/redtoad/xcom-editor/internal"
 )
 
 const maxBases = 8
@@ -55,7 +56,7 @@ type BaseData struct {
 
     // 00-0E: BaseData Name, pretty obvious
     // 0F: Presumably the Null character if the BaseData Name uses all 15 characters
-	Name string `struct:"[16]byte"`
+	Name internal.NullString `struct:"[16]byte"`
 
 	// Logical values for the detection capabilities:
 	//
@@ -90,10 +91,12 @@ type BaseData struct {
 	// 60-11E inventory
 	Inventory [96]int `struct:"[96]int16"`
 
-    // 0120: Active/Inactive BaseData. Inactive entries have a value of 1. Active entries have a value of 0. Creating a new base will overwrite the first inactive entry. If a base is dismantled, the only change to the record is this value so it is possible to restore a dismantled base (Access lift removed) by restoring this value to 0. --SeulDragon 12:24, 11 July 2008 (PDT)
-	Active bool `struct:"int8,invertedbool"`
-
-	// 0121~0123: 0120 is stored as an integer. These fields are the unused portion of that integer.
+    // 0120-0123: Active/Inactive BaseData, stored as a 4-byte integer.
+	// Inactive entries have a value of 1. Active entries have a value of 0.
+	// Creating a new base will overwrite the first inactive entry. If a base is
+	// dismantled, the only change to the record is this value so it is possible
+	// to restore a dismantled base (Access lift removed) by restoring this value to 0.
+	Active bool `struct:"int32,invertedbool"`
 }
 
 

internal/geoscape/saveinfo_dat.go

@@ -3,17 +3,21 @@ package geoscape
 // This file is present in any save.
 // https://www.ufopaedia.org/index.php/SAVEINFO.DAT
 
-import "time"
+import (
+	"time"
+
+	"github.com/redtoad/xcom-editor/internal"
+)
 
 // SaveinfoFile contains name and game time of the saved game.
 // This file is 40 bytes long, no separate entries.
 type SaveinfoFile struct {
 
 	// 0-1	0x00-0x01	Ignore this if the file is not in the missdat folder. If 0, then this is a savegame made on the beginning of a new battlescape game. If 1, then check DIRECT.DAT to see where which save slot to load from.
-	_ bool `struct:"int16"`
+	MissdatFlag bool `struct:"int16"`
 
 	// 2-27	0x02-0x1D	This a 26 byte null terminated string, which details the name of the save file. The name may be 25 characters long; the final byte is always of value 0. A 0 also marks the end of the save name, should it not use all 25 characters.
-	Name string `struct:"[26]byte"`
+	Name internal.NullString `struct:"[26]byte"`
 
 	// 28-29	0x1C-0x1D	The current year.
 	Year int `struct:"int16"`

internal/geoscape/saveinfo_dat_test.go

@@ -37,3 +37,52 @@ func TestSaveinfoFile_Time(t *testing.T) {
 		})
 	}
 }
+
+func TestSaveinfoFile_Name(t *testing.T) {
+	tests := []struct {
+		hex  string
+		want string
+	}{
+		{
+			"01005465737400000000000000000000000000000000000000000000cf0703000d00040014000000",
+			"Test",
+		},
+		{
+			"010057656c6c206f6e20746865207761790000000000000000000000cf0703000e0002002f000000",
+			"Well on the way",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.hex, func(t *testing.T) {
+			data, err := loadHex(tt.hex)
+			assert.NoError(t, err)
+
+			var info geoscape.SaveinfoFile
+			err = restruct.Unpack(data, binary.LittleEndian, &info)
+			assert.NoError(t, err, "could not unpack test data: %v", err)
+
+			assert.Equal(t, tt.want, info.Name.String())
+		})
+	}
+}
+
+func TestSaveinfoFile_RoundTrip(t *testing.T) {
+	tests := []string{
+		"01005465737400000000000000000000000000000000000000000000cf0703000d00040014000000",
+		"010057656c6c206f6e20746865207761790000000000000000000000cf0703000e0002002f000000",
+	}
+	for _, hex := range tests {
+		t.Run(hex, func(t *testing.T) {
+			data, err := loadHex(hex)
+			assert.NoError(t, err)
+
+			var info geoscape.SaveinfoFile
+			err = restruct.Unpack(data, binary.LittleEndian, &info)
+			assert.NoError(t, err, "could not unpack test data: %v", err)
+
+			encoded, err := restruct.Pack(binary.LittleEndian, &info)
+			assert.NoError(t, err, "could not pack test data: %v", err)
+			assert.Equal(t, data, encoded)
+		})
+	}
+}

internal/geoscape/soldier_dat.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 
 	"github.com/go-restruct/restruct"
+	"github.com/redtoad/xcom-editor/internal"
 )
 
 const maxSoldiers = 250
@@ -115,7 +116,7 @@ type SoldierData struct {
 	// stored directly instead of going through Windows objects. The end of the current
 	// name is a null byte; garbage can be present after that (ends of longer previous
 	// names, etc.).
-	Name string `struct:"[25]byte"`
+	Name internal.NullString `struct:"[25]byte"`
 
 	// 41 / 29 (various): Always 0 except for existing soldiers being transferred, in
 	// which case it equals the LOC.DAT value for destination base. New recruits on

internal/geoscape/soldier_dat_test.go

@@ -70,6 +70,9 @@ func TestUnpackSoldier(t *testing.T) {
 			err = restruct.Unpack(data, binary.LittleEndian, &soldier)
 			assert.NoError(t, err, "could not unpack test data: %v", err)
 
+			assert.Equal(t, tt.expected.Name.String(), soldier.Name.String())
+			// Name carries null padding from binary data; normalize before struct comparison.
+			soldier.Name = tt.expected.Name
 			assert.Equal(t, tt.expected, soldier)
 
 		})

internal/types.go

@@ -1,17 +1,18 @@
 package internal
 
 import (
-	"encoding/binary"
 	"strings"
 )
 
-// NullString is a null byte terminated string.
+// NullString is a null byte terminated string. When read from binary data via
+// restruct, the full fixed-size byte field (including null bytes) is stored.
+// Use String() to get the value truncated at the first null byte.
 type NullString string
 
-// Unpack implements the restruct.Unpacker interface.
-func (s *NullString) Unpack(buf []byte, order binary.ByteOrder) ([]byte, error) {
-	str := string(buf)
-	nul := strings.IndexByte(str, 0x0)
-	*s = NullString(str[0:nul])
-	return []byte{}, nil
+// String returns the string value, truncated at the first null byte.
+func (s NullString) String() string {
+	if nul := strings.IndexByte(string(s), 0x0); nul >= 0 {
+		return string(s[:nul])
+	}
+	return string(s)
 }

internal/types_test.go

@@ -10,7 +10,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestNullString_Unpack(t *testing.T) {
+func TestNullString_String(t *testing.T) {
 
 	type nullStringStruct struct {
 		Name internal.NullString `struct:"[26]byte"`
@@ -35,12 +35,47 @@ func TestNullString_Unpack(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(fmt.Sprintf("%x", tt.bytes), func(t *testing.T) {
 			var value nullStringStruct
-			_ = restruct.Unpack(tt.bytes, binary.LittleEndian, &value)
-			assert.Equal(t, tt.want, string(value.Name))
+			err := restruct.Unpack(tt.bytes, binary.LittleEndian, &value)
+			assert.NoError(t, err)
+			assert.Equal(t, tt.want, value.Name.String())
 		})
 	}
 }
 
+func TestNullString_StringNoNull(t *testing.T) {
+	// When no null byte is present, the full string is returned.
+	s := internal.NullString("ABCDEF")
+	assert.Equal(t, "ABCDEF", s.String())
+}
+
+func TestNullString_MultiField(t *testing.T) {
+
+	// Verify NullString works correctly in a struct with multiple fields.
+	type multiFieldStruct struct {
+		ID   int16               `struct:"int16"`
+		Name internal.NullString `struct:"[10]byte"`
+		Age  int16               `struct:"int16"`
+	}
+
+	buf := []byte{
+		0x01, 0x00, // ID = 1
+		0x41, 0x6c, 0x69, 0x63, 0x65, 0x00, 0x00, 0x00, 0x00, 0x00, // Name = "Alice\0\0\0\0\0"
+		0x1e, 0x00, // Age = 30
+	}
+
+	var value multiFieldStruct
+	err := restruct.Unpack(buf, binary.LittleEndian, &value)
+	assert.NoError(t, err)
+	assert.Equal(t, int16(1), value.ID)
+	assert.Equal(t, "Alice", value.Name.String())
+	assert.Equal(t, int16(30), value.Age)
+
+	// Round-trip: packing the unpacked struct must produce identical bytes.
+	encoded, err := restruct.Pack(binary.LittleEndian, &value)
+	assert.NoError(t, err)
+	assert.Equal(t, buf, encoded)
+}
+
 func TestNullString_Pack(t *testing.T) {
 
 	type nullStringStruct struct {

savegame/bases.go

@@ -14,7 +14,7 @@ type Base struct {
 }
 
 func (b *Base) Name() string {
-	return b.game.baseFile.Bases[b.offset].Name
+	return b.game.baseFile.Bases[b.offset].Name.String()
 }
 
 func (b *Base) Coord() Coord {
@@ -81,7 +81,7 @@ func (game *Savegame) CompleteConstructions() {
 
 func (game *Savegame) Base(offset int) *Base {
 	base := game.baseFile.Bases[offset]
-	if len(base.Name) > 0 {
+	if base.Name.String() != "" {
 		return &Base{
 			offset: offset,
 			game:   game,
@@ -93,7 +93,7 @@ func (game *Savegame) Base(offset int) *Base {
 func (game *Savegame) Bases() []*Base {
 	bases := make([]*Base, 0)
 	for idx, base := range game.baseFile.Bases {
-		if len(base.Name) > 0 {
+		if base.Name.String() != "" {
 			bases = append(bases, game.Base(idx))
 		}
 	}

savegame/metadata.go

@@ -17,7 +17,7 @@ func (game *Savegame) loadMetadata() error {
 
 // Title returns the savegame title.
 func (game *Savegame) Title() string {
-	return game.meta.Name
+	return game.meta.Name.String()
 }
 
 // Time returns the game time.

savegame/soldiers.go

@@ -17,7 +17,7 @@ type Soldier struct {
 }
 
 func (s *Soldier) Name() string {
-	return s.game.soldierFile.Soldiers[s.offset].Name
+	return s.game.soldierFile.Soldiers[s.offset].Name.String()
 }
 
 type Rank int
@@ -65,7 +65,7 @@ func (s *Soldier) Craft() *Craft {
 
 func (s *Soldier) IsDead() bool {
 	data := s.game.soldierFile.Soldiers[s.offset]
-	return data.Rank == geoscape.DeadOrUnused && strings.TrimSpace(data.Name) != ""
+	return data.Rank == geoscape.DeadOrUnused && strings.TrimSpace(data.Name.String()) != ""
 }
 
 func (s *Soldier) IsWounded() bool {
@@ -106,7 +106,7 @@ func (game *Savegame) Soldiers() []*Soldier {
 		// When soldiers die, their Rank is set to geoscape.DeadOrUnused any can
 		// be overwritten. Therefore we assume that any non empty name marks a
 		// soldier's entry.
-		if data.Name == "" {
+		if data.Name.String() == "" {
 			continue
 		}
 		soldiers = append(soldiers, &Soldier{