[Firestore][Crash] nullpointer dereference in transactions

[Frank-Schmutz]

Please fill in the following fields:

Unity editor version: 2019.3.15f1
Firebase Unity SDK version: 6.15.2
Source you installed the SDK (.unitypackage or Unity Package Manager): .unitypackage
Firebase plugins in use (Auth, Database, etc.): Firestore
Additional SDKs you are using (Facebook, AdMob, etc.): None
Platform you are using the Unity editor on (Mac, Windows, or Linux): Windows
Platform you are targeting (iOS, Android, and/or desktop): Android
Scripting Runtime (Mono, and/or IL2CPP): Mono

Please describe the issue here:

Firestore crashes when executing (even simple) transactions. Here is the relevant code used to reproduce these crashes:

Document Structure:

[FirestoreData]
public class User
{
    [FirestoreProperty] public string username { get; set; }
    [FirestoreProperty] public int counter { get; set; }
}

Transaction code:

FirebaseFirestore db = FirebaseFirestore.DefaultInstance;
DocumentReference userRef = db.Collection("users").Document(username);
db.RunTransactionAsync(transaction =>
{
	return transaction.GetSnapshotAsync(userRef).ContinueWith((task) =>
	{
		DocumentSnapshot snapshot = task.Result;
		userRef = snapshot.Reference;
		Debug.LogError("snapshot.Reference: \"" + userRef.ToString() +
				  "\"; snapshot.Id: \"" + userRef.Id +
				  "\"; snapshot.Path: \"" + userRef.Path + "\"");

		if (snapshot.Exists)
		{
			User user = snapshot.ConvertTo<User>();
			user.counter += 1;

			userRef = snapshot.Reference;
			Debug.LogError("snapshot.Reference: \"" + userRef.ToString() +
					  "\"; snapshot.Id: \"" + userRef.Id +
					  "\"; snapshot.Path: \"" + userRef.Path + "\"");

			transaction.Set(userRef, user);
		}
	});
});

The crashes can happen both in the Editor or on Android (We did not try it on iOS). The crash is triggered by a signal 11 (SIGSEGV), code 1 (SEGV_MAPERR) caused by a null pointer dereference. There are multiple library calls that occasionally trigger this:

1. Calling the GetSnapshotAsync method
   Full StackTrace: GetSnapshotAsync_stacktrace.txt

2. The reference return when calling snapshot.Reference is incorrect, which is used by TransactionSet
   Full StackTrace: Reference_TransactionSet_stacktrace.txt
   Here we can see that the reference was at the root of the database and missing the collection and document parts. Also it was correct when calling snapshot.Reference the first time but not the second time.
   
   

3. Calling the ToDictionary method on a DocumentSnapshot
   We could not reproduce this error on the minimal code written above but this occurs regularly on larger documents
   

4. Calling the ConvertTo method on a DocumentSnapshot
   As for the last error, e could not reproduce this on the minimal code written above but this occurs regularly on larger documents
   

Please answer the following, if applicable:

Have you been able to reproduce this issue with just the Firebase Unity quickstarts (this GitHub project)?

No we only reproduced it with the minimal code samples written above.

What's the issue repro rate? (eg 100%, 1/5 etc):

Depends on the cause. The most frequent one is the incorrect reference which on the minimal code written above happens once every ~100 transactions. When using larger documents those issues arise much more frequently (about 1/25 to 1/50 transactions there is a crash).

[kyleger]

Any updates concerning this issue? These crashes are making Firestore unusable.

[wu-hui]

Hi @kyleger

I am very sorry to get back to you so late. I have been having issues with my Windows laptop and Unity license.

I am finally able to try out your snippet, and unfortunately I am unable to reproduce the crash with Editor. It successfully increased the counter after run, given that the document already exists.

My Unity Editor version is 2019.4.8f1. I basically added your snippet to the firestore quickstart App to reproduce.

I am trying to test this on Android now, but would like to share my findings with you, because it is long overdue.

OTOH, can you try provide a minimal repro in a git repo, or something like that, hopefully I have better luck trying that.

Thanks

[Frank-Schmutz]

Hi @wu-hui

Here is the minimal code we used to reproduce this issue. It contains a single C# monobehaviour script that we attach to an empty game object in the scene. The script simply does the checkandfixdependencies and then runs the transaction every 2 seconds.
Hope you have better luck with this. In the meantime we will try to reproduce the issue on the 2019.4.8f1 version both in the editor and on android.
minimal_code.zip

Thanks

[kyleger]

Hi @wu-hui ,
Thank you for the update. Using the code sample uploaded by @Frank-Schmutz you should be able to reproduce the error in the editor. Depending on luck / computer you are using, it might take between 10-1000 transactions to crash. However, in our runs it always eventually ends up crashing, no matter the computer OS, firebase version or Editor version.
Thanks for looking into this.

[wu-hui]

Thank you for the script. I have tried that, and currently the counter is increased up to 100 and still not crashed, with Editor.

I am going to try Android. Let me know if you had any luck with a different version of Unity.

[kyleger]

Did you manage to reproduce the crash on Android? Please try to reach 1000 transactions on the editor. The crash sometimes occurs quite late. Once it has occurred once, it somehow becomes more frequent.

I have managed to reproduce the crash with the 2019.4.8f1 Unity Editor version.

[wu-hui]

Yeah, I still have no luck with Editor, I did try reach 1000 transactions.

I was having some issues with my Android build yesterday, will try again today.

[Frank-Schmutz]

We noticed the crash occurs much more frequently with larger documents. So we added a list of 10'000 integers to the document I previously sent you and the crash occurs after less than 20 transactions every time.

Can you please try with this new version?

Here is the new sample code:
minimal_code.zip

[kyleger]

Have you managed to reproduce the error using the above code? If so, do you have an idea of where the bug is coming from and if there is a possible workaround/fix?

With the most recent 'minimal_code' we have a 100% crash rate in under 20 transactions (often in the first 2-5). This is with multiple computers/Unity versions/firebase versions.

Thanks for looking into this!

[wu-hui]

Sorry for the delay.

Yes, the new repro works, and I can see the crash. Will get back to you when I have something.

[ll-II]

Hi @wu-hui,
Any updates regarding this issue? I'm facing similar problems

Do you know when a fix will be available?
Thanks a lot !

[dconeybe]

Update: I've taken over this issue from @wu-hui for now. We are able to reproduce the issue and are actively debugging to find the root cause; however, so far the root cause has eluded us. When we find a fix it will be available in the next release; however, we have no ETA on either. Hopefully there will be a workaround we can publish. We will update this thread as soon as there is something concrete to report.

@AP-EPFL, thanks for letting us know that this issue is affecting you as well.

[wu-hui]

So I have finally been able to identify the root cause, and a fix will be released in 6.16.0.

Some details if you are curious: what happened is a use-after-free issue, a C++ object is references by a transient C# object, and another C# object. When .NET gc kicks in, it delete the transient C# object, which also delete the C++ object. Then when you reference from the other C# object, this will happen. The underlying issue is the C# object should own the C++ object properly.

The randomness comes from when the GC runs, that is why when you add padding it will reliably reproduce: your transaction runs longer, and GC will kick in in the middle of transaction.

We are conducting an audit to see if there are other places where this could happen. Before 6.16.0, reading snapshots from within a transaction is basically broken, sorry!

[kyleger]

Thanks for finding the bug, seems like it caused a few headaches...

Do you have an approximate ETA on 6.16.0 (days/weeks/months)?