FIX(overlay): Add strlen check of XDG_RUNTIME_DIR before strcpy

[augustus-7613]

Add a length check of the XDG_RUNTIME_DIR environment variable before using strcpy, to ensure it fits inside the ctx->saName.sun_path buffer. Before this commit, if the value from the XDG_RUNTIME_DIR was above the size of the ctx->saName.sun_path buffer, it would overflow the buffer.

[coderabbitai]

Walkthrough

A length validation check is introduced to the XDG_RUNTIME_DIR path handling in overlay.c. The modification ensures that before using the environment variable to construct a Unix socket path, both the pointer is non-NULL and the string length does not exceed the buffer size of ctx->saName.sun_path. This prevents potential buffer overflow during path concatenation. The control flow remains unchanged: if the validation passes, the code sets PF_UNIX and constructs the path as before; otherwise, it falls back to using the user's home directory path.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main security fix: adding a strlen check for XDG_RUNTIME_DIR before strcpy to prevent buffer overflow.
Description check	✅ Passed	The description explains the buffer overflow vulnerability and the fix applied, but does not include the commit guidelines checklist item from the template.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

overlay_gl/overlay.c (1)

162-166: Missing buffer overflow protection for home directory path.

The fallback path using the home directory has no length validation before strcpy and strcat. If the home directory path is long, this will overflow ctx->saName.sun_path.

Apply this diff to add bounds checking:

 	} else if (home) {
-		ctx->saName.sun_family = PF_UNIX;
-		strcpy(ctx->saName.sun_path, home);
-		strcat(ctx->saName.sun_path, "/.MumbleOverlayPipe");
+		if (strlen(home) + 19 < sizeof(ctx->saName.sun_path)) {
+			ctx->saName.sun_family = PF_UNIX;
+			strcpy(ctx->saName.sun_path, home);
+			strcat(ctx->saName.sun_path, "/.MumbleOverlayPipe");
+		}
 	}

Or use snprintf for both paths:

 	} else if (home) {
 		ctx->saName.sun_family = PF_UNIX;
-		strcpy(ctx->saName.sun_path, home);
-		strcat(ctx->saName.sun_path, "/.MumbleOverlayPipe");
+		int ret = snprintf(ctx->saName.sun_path, sizeof(ctx->saName.sun_path), "%s/.MumbleOverlayPipe", home);
+		if (ret < 0 || (size_t)ret >= sizeof(ctx->saName.sun_path)) {
+			ctx->saName.sun_family = 0;
+		}
 	}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 523c13f and 1f6b3af.

📒 Files selected for processing (1)

• overlay_gl/overlay.c (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: PR (Windows_x64)
• GitHub Check: PR (Linux)
• GitHub Check: PR (Translations)
• GitHub Check: PR (macOS)
• GitHub Check: PR (Docs)
• GitHub Check: freebsd

[Krzmbrzl]

What's the + 18 for?

[augustus-7613]

It is the length of the string "/MumbleOverlayPipe". This also needs to fit into the ctx->saName.sun_path buffer.
The + 19 is the same just for the string "/.MumbleOverlayPipe"

[Krzmbrzl]

I see. I guess it would be better to use strlen on the corresponding string constant instead of using a "magic number".
And if no such constant exists yet, we should create one

[augustus-7613]

Yea, I understand. I thought about using strlen but then the strings /MumbleOverlayPipe and /.MumbleOverlayPipe would have to be written twice. Neither option is pretty I guess.

I can move both string into their own char * variables, and use that variable in strlen and strcat if that sounds reasonable to you?

[Krzmbrzl]

Yes, that's hat I had in mind 👍

[augustus-7613]

Why did the builds fail?

[Hartmnt]

Could you please rebase your branch against the current master branch and squash your commits into one?

[Krzmbrzl]

Note, you need to first fetch from our repo (upstream) to make sure you got the latest changes from our master branch. Then rebase against that branch instead of your local version of master (which seems to be a couple of commits behind ours - one of those commits fixes the FreeBSD CI issue)

I.e. do

git pull --rebase https://github.com/mumble-voip/mumble.git master

from your repo while having your branch checked out

[augustus-7613]

Okey, thanks. I think this should work now? Sorry for the extra steps here

[Hartmnt]

@Krzmbrzl Is this good to go?