Fixing Brackeys' save & load system

Oliver Oliver • Published 3 years ago Updated 2 years ago


Recent news has hit that BinaryFormatter has a major security flaw, and should be removed from production use as soon as possible. This guide will cover an alternative approach to creating a save & load system for your Unity game.

This guide works under the assumption that you have watched Brackeys' tutorial on creating a save & load system in Unity, which you can do here:

Watched it? Understand it? Perfect. Forget everything about it.

Well okay, not everything. Remember that part at the start of the video where he said he'll cover how to save game data “locally and safely?”

I'm here to destroy your dreams by pointing out that Microsoft has recently released this statement regarding BinaryFormatter:

Warning

The BinaryFormatter type is dangerous and is not recommended for data processing. Applications should stop using BinaryFormatter as soon as possible, even if they believe the data they're processing to be trustworthy. BinaryFormatter is insecure and can't be made secure.

So what can we do to fix it? What are the alternatives?

Fixing the PlayerData model

We'll start by taking a look at the PlayerData type that Brackeys defined in his tutorial.

[Serializable]
public class PlayerData
{
    public int level;
    public int health;
    public float[] position;

    public PlayerData(Player player)
    {
        level = player.level;
        health = player.health;

        position = new float[3];
        position[0] = player.transform.position.x;
        position[1] = player.transform.position.y;
        position[2] = player.transform.position.z;
    }
}

First thing is first, we should fix this code style. To make this work we need to promote these fields to properties. While we're at it let's also fix their naming style so that they are in PascalCase.

[Serializable]
public class PlayerData
{
    public int <mark>Level { get; set; }</mark>
    public int <mark>Health { get; set; }</mark>
    public float[] <mark>Position { get; set; }</mark>

    public PlayerData(Player player)
    {
        Level = player.level;
        Health = player.health;

        Position = new float[3];
        Position[0] = player.transform.position.x;
        Position[1] = player.transform.position.y;
        Position[2] = player.transform.position.z;
    }
}

The next step would be to move the constructor to the top of the class because constructors should be defined above properties, but instead I'm going to change it to a static factory method named FromPlayer and that way it can remain where it is. It will return a new instance of PlayerData built from the Player parameter, using an initialiser.

[Serializable]
public class PlayerData
{
    public int Level { get; set; }
    public int Health { get; set; }
    public float[] Position { get; set; }

    public static PlayerData FromPlayer(Player player)
    {
        <mark>return new PlayerData</mark>
        <mark>{</mark>
        <mark>    Level = player.level,</mark>
        <mark>    Health = player.health,</mark>
        <mark>    Position = new float[]</mark>
        <mark>    {</mark>
        <mark>        player.transform.position.x,</mark>
        <mark>        player.transform.position.y,</mark>
        <mark>        player.transform.position.z</mark>
        <mark>    }</mark>
        <mark>};</mark>
    }
}

For now this is fine. There are some other things we can do to improve it but they are not important for the purpose of this guide. These changes are also largely unrelated to working around the security issues inherent to BinaryFormatter, but the reason for them will become clear in a future blog post.

Fixing the SaveSystem class

Okay…

public static class SaveSystem
{
    public static void SavePlayer(Player player)
    {
        BinaryFormatter formatter = new BinaryFormatter();
        string path = Application.persistentDataPath + "/player.fun";
        FileStream stream = new FileStream(path, FileMode.Create);
        
        PlayerData data = new PlayerData(player);
        
        formatter.Serialize(stream, data);
        stream.Close();
    }

    public static PlayerData LoadPlayer()
    {
        string path = Application.persistentDataPath + "/player.fun";
        if (File.Exists(path))
        {
            BinaryFormatter formatter = new BinaryFormatter();
            FileStream stream = new FileStream(path, FileMode.Open);
            
            PlayerData data = formatter.Deserialize(stream) as PlayerData;
            stream.Close();
            
            return data;
        }
        else
        {
            Debug.LogError("Save file not found in " + path);
            return null;
        }
    }
}

Where to begin? Oh boy.

First, let's fix the PlayerData instantation in the SavePlayer method. We need to call the static FromPlayer method written earlier. This will fix the compile errors we introduced.

public static void SavePlayer(Player player)
{
    BinaryFormatter formatter = new BinaryFormatter();
    string path = Application.persistentDataPath + "/player.fun";
    FileStream stream = new FileStream(path, FileMode.Create);

    <mark>PlayerData data = PlayerData.FromPlayer(player);</mark>

    formatter.Serialize(stream, data);
    stream.Close();
}

Next we will take the advice that Brackeys gave. This comes in two parts.

The first is to separate out the path variable from both of the methods so we don't have duplicate code.

The second is to avoid the use of using a literal / for the directory separator, since on Windows the directory separator character is actually \ whereas it is / on Unix systems. We can resolve this by using the field Path.DirectorySeparatorChar, but a better way would be to call Path.Combine. This will combine a variable number of strings together into a full directory path. Pass in Application.persistentDataPath as the first argument, and the string player.fun as the second.

public static class SaveSystem
{
    <mark>private static readonly string path = Path.Combine(Application.persistentDataPath, "player.fun");</mark>
    
    public static void SavePlayer(Player player)
    {
        BinaryFormatter formatter = new BinaryFormatter();
        FileStream stream = new FileStream(<mark>path</mark>, FileMode.Create);

        PlayerData data = PlayerData.FromPlayer(player);

        formatter.Serialize(stream, data);
        stream.Close();
    }

    public static PlayerData LoadPlayer()
    {
        if (File.Exists(path))
        {
            BinaryFormatter formatter = new BinaryFormatter();
            FileStream stream = new FileStream(<mark>path</mark>, FileMode.Open);

            PlayerData data = formatter.Deserialize(stream) as PlayerData;
            stream.Close();

            return data;
        }
        else
        {
            Debug.LogError("Save file not found in " + path);
            return null;
        }
    }
}

Saying goodbye to BinaryFormatter

As mentioned earlier, BinaryFormatter is insecure - and cannot be made secure by the .NET team. But what could we use instead? I'll cover two possible alternatives.

The first is a combination of BinaryWriter and BinaryReader, which will be responsible for saving and loading respectively.

Fixing SavePlayer

Most of the code in this method is okay; we still need a FileStream and we still need our PlayerData instance. We just need to swap out BinaryFormatter for something better. Let's instead change it to a BinaryWriter whose constructor accepts output stream. This means it will need to be declared after we open the file stream.

public static void SavePlayer(Player player)
{
    FileStream stream = new FileStream(path, FileMode.Create);
    <mark>BinaryWriter writer = new BinaryWriter(stream);</mark>
    
    PlayerData data = PlayerData.FromPlayer(player);

    stream.Close();
}

Now we need to write our data to the stream. One of the benefits that BinaryFormatter provided was letting us serialize and deserialize using a single method call, keeping our code short and orderly. Unfortunately, using BinaryWriter means we have to sacrifice this simplicity because BinaryWriter is only capable of writing built-in types.

This means that our 2 integer variables Level and Health, plus our float array Position, must be written one at a time - by you the programmer. This isn't difficult, just time consuming.

Use the Write method to write each of the values in PlayerData.

public static void SavePlayer(Player player)
{
    FileStream stream = new FileStream(path, FileMode.Create);
    BinaryWriter writer = new BinaryWriter(stream);
    
    PlayerData data = PlayerData.FromPlayer(player);
    writer.Write(<mark>data.Level</mark>);
    writer.Write(<mark>data.Health</mark>);
    writer.Write(<mark>data.Position[0]</mark>);
    writer.Write(<mark>data.Position[1]</mark>);
    writer.Write(<mark>data.Position[2]</mark>);

    stream.Close();
}

You'll notice we even have to write the array elements one at a time. This is because Write does not accept a float array, but it does accept a single float, so we are left to write each float separately. This starts to become very tedious very quickly, and later in this article I will explain some QoL improvements to save us headaches down the road.

For now though, this method is essentially done. Let's move over and fix LoadPlayer!

Fixing LoadPlayer

So how do we extract out BinaryFormatter here? Why BinaryReader of course! (Bet you didn't see that coming.)

Declare a new BinaryReader after opening the file stream (replacing the instantiation of BinaryFormatter), and replace the call to formatter.Deserialize() with an instantiation of a new PlayerData.

public static PlayerData LoadPlayer()
{
    if (File.Exists(path))
    {
        FileStream stream = new FileStream(path, FileMode.Open);
        <mark>BinaryReader reader = new BinaryReader(stream);</mark>
            
        PlayerData data = new PlayerData();

        stream.Close();

        return data;
    }
    else
    {
        Debug.LogError("Save file not found in " + path);
        return null;
    }
}

Before we start reading the file, there is a very important piece of information I need to cover. The order in which we wrote the values in SavePlayer, must exactly match that of reading them in LoadPlayer.

First we wrote the integer level, then the integer health, and then the 3 float array elements for the position. We must read them in the same order, or we will get very weird side effects. You won't necessarily get any compile errors, but put simply: BinaryReader doesn't care what the bytes in the file are supposed to represent. It doesn't know that the first 4 bytes are supposed to be the Level integer, that is just what we are deciding they should be. If you are sloppy, reading them in the wrong order can be a cause for some very strange memory issues - even possibly crashing your game.

So now that I've drilled that positively frightening tidbit into you, let's read the player data!

To read an integer from the file, call the ReadInt32 method on the BinaryReader. Store the result of this call into the Level value in data. Do this again for Health.

public static PlayerData LoadPlayer()
{
    if (File.Exists(path))
    {
        FileStream stream = new FileStream(path, FileMode.Open);
        BinaryReader reader = new BinaryReader(stream);
            
        PlayerData data = new PlayerData();
        <mark>data.Level = reader.ReadInt32();</mark>
        <mark>data.Health = reader.ReadInt32();</mark>

        stream.Close();

        return data;
    }
    else
    {
        Debug.LogError("Save file not found in " + path);
        return null;
    }
}

The last thing we need is the position in the form of 3 distinct floats. Create a new array for the position, and call the ReadSingle method to assign the elements in the array in order.

public static PlayerData LoadPlayer()
{
    if (File.Exists(path))
    {
        FileStream stream = new FileStream(path, FileMode.Open);
        BinaryReader reader = new BinaryReader(stream);
            
        PlayerData data = new PlayerData();
        data.Level = reader.ReadInt32();
        data.Health = reader.ReadInt32();

        <mark>data.Position = new float[3];</mark>
        <mark>data.Position[0] = reader.ReadSingle();</mark>
        <mark>data.Position[1] = reader.ReadSingle();</mark>
        <mark>data.Position[2] = reader.ReadSingle();</mark>

        stream.Close();

        return data;
    }
    else
    {
        Debug.LogError("Save file not found in " + path);
        return null;
    }
}

That is it! We've now removed the security flaw that BinaryFormatter presented us. Another benefit of this is in doing so, we've also made our save file completely portable. If you decided to rewrite your game in another programming language, you would be able to read the same save-file format as you did in C#.

If you are happy with this solution, you are free to call it quits here. We're done. But next, I will talk about a way to make reading and writing arrays a lot easier.

Serialization QoL improvements

As I mentioned earlier, reading and writing arrays starts to get very tedious very quickly. If the arrays are particularly large, you would end up having to read and write the every single element separately which could take potentially hundreds of lines of assignments and method calls. That simply will not do.

First, let's start off by improving SavePlayer.

Fixing SavePlayer even more

Earlier on I told you to write each element of the array separately:

writer.Write(data.Position[0]);
writer.Write(data.Position[1]);
writer.Write(data.Position[2]);

But what if the array is null? What if you wanted to write an array whose length you did not know? We know the array has a length of 3 in this case, but we are not always so fortunate. We can solve this by using a foreach loop!

First, we need to write the length of the array to the file, so call Write and pass in the array length. The reason for this will become clear when I explain how to improve LoadPlayer, so bear with me.

Immediately after, loop through the array and write each element.

writer.Write(data.Position.Length);
foreach (float element in data.Position)
{
    writer.Write(element);
}

This solves the variable-length problem, but it will still throw an error if the array is null. To work around that, simply check if the array is null and if it is, indicate that the array is “empty” by writing a single 0 to the file. Surround the code we just wrote in an else branch.

if (data.Position == null)
{
    <mark>writer.Write(0);</mark>
}
else
{
    writer.Write(data.Position.Length);
    foreach (var value in data.Position)
    {
        writer.Write(value);
    }
}

The full SavePlayer method should now look like this:

public static void SavePlayer(Player player)
{
    FileStream stream = new FileStream(path, FileMode.Create);
    BinaryWriter writer = new BinaryWriter(stream);
    
    PlayerData data = PlayerData.FromPlayer(player);
    writer.Write(data.Level);
    writer.Write(data.Health);

    if (data.Position == null)
    {
        writer.Write(0);
    }
    else
    {
        writer.Write(data.Position.Length);
        foreach (var value in data.Position)
        {
            writer.Write(value);
        }
    }

    stream.Close();
}

Fixing LoadPlayer even more

Now that SavePlayer knows how to write an array of any length, how can we have LoadPlayer read it? For instance, when we wrote new float[3] to assign the Position value, how can we change this so we know how large the array needs to be, supporting any size?

This is why we wrote the length of the array in SavePlayer! To start off, let's call ReadInt32 to fetch that next integer after Health, this will be the length we need. We can then create a new float array, using that length.

int length = reader.ReadInt32();
data.Position = new float[length];

And now to assign the elements, we can use another loop (this time a for loop) to read as many values as we need.

for (int index = 0; index < length; index++)
{
    data.Position[index] = reader.ReadSingle();
}

And there we have it. This is how we can accept variable-length arrays when writing and reading, using BinaryWriter and BinaryReader. Your full LoadPlayer method should now look something like this:

public static PlayerData LoadPlayer()
{
    if (File.Exists(path))
    {
        FileStream stream = new FileStream(path, FileMode.Open);
        BinaryReader reader = new BinaryReader(stream);
            
        PlayerData data = new PlayerData();
        data.Level = reader.ReadInt32();
        data.Health = reader.ReadInt32();

        int length = reader.ReadInt32();
        data.Position = new float[length];
        <mark>for (int index = 0; index < length; index++)</mark>
        <mark>{</mark>
        <mark>  data.Position[index] = reader.ReadSingle();</mark>
        <mark>}</mark>

        stream.Close();

        return data;
    }
    else
    {
        Debug.LogError("Save file not found in " + path);
        return null;
    }
}

A final note on LoadPlayer

I was never really a fan of how the if-else is written here. This whole structure screams out guard clause. As a code style improvement, we should instead validate the file exists by having an early return (also known as fail-fast) at the head of the method. Doing this will also improve readability by reducing the need to nest.

public static PlayerData LoadPlayer()
{
    <mark>if (!File.Exists(path))</mark>
    <mark>{</mark>
    <mark>    Debug.LogError("Save file not found in " + path);</mark>
    <mark>    return null;</mark>
    <mark>}</mark>

    FileStream stream = new FileStream(path, FileMode.Open);
    BinaryReader reader = new BinaryReader(stream);

    PlayerData data = new PlayerData();
    data.Level = reader.ReadInt32();
    data.Health = reader.ReadInt32();

    int length = reader.ReadInt32();
    data.Position = new float[length];
    for (int index = 0; index < length; index++)
    {
        data.Position[index] = reader.ReadSingle();
    }

    stream.Close();

    return data;
}

Handling large data structures

We've covered how to handle variable length arrays in our serialization models. But what if the models themselves are particularly large? What if we have 10 or 20 properties we need to save? What if those properties aren't built-in types but are instead custom classes? We will end up having so many calls to Write and Read methods that our save system becomes unmaganable.

Fear not, there is a solution. It is the second alternative that I hinted at earlier. I've written an entirely new blog post which outlines the fundamentals of creating a scalable save & load system from scratch using Protocol Buffers!




4 legacy comments

Legacy comments are comments that were posted using a commenting system that I no longer use. This exists for posterity.

Χρυσοβαλάντης Κρεμουλίνος Χρυσοβαλάντης Κρεμουλίνος • 3 years ago

if i try this it doesnt recognise the playerdata fromplayer function…

Oliver Booth Oliver Booth • 3 years ago

Then that means you missed something. Ensure it is marked public and static

Anton Bissø Stausholm Anton Bissø Stausholm • 2 years ago

Great work! How does the actual save and load functions where you have to get the varibles through playerdata and then to save and load system and eventually back to the original varibles work? With that i mean https://paste.myst.rs/xola0vly this part of brackey's turtorial

Isaac R Isaac R • 9 months ago

Hi, I have the same problem as @Χρυσοβαλάντης Κρεμουλίνος . PlayerData is definitely both public and static but i keep getting the errors;

Assets\Scripts\SaveSystem.cs(19,16): error CS0246: The type or namespace name ‘PlayerData’ could not be found (are you missing a using directive or an assembly reference?)

Assets\Scripts\Data.cs(166,16): error CS0246: The type or namespace name ‘PlayerData’ could not be found (are you missing a using directive or an assembly reference?)

Not to be rude but a solution soon would be greatly appreciated. I'm making a quiz for Christmas and I NEED to be done by the 24th. Thanks

P.S I fixed it, I didn't realize the script had to be called PlayerData as well