🐛 [Bug]: server: protect field access with lock to avoid possible data race

[ririli]

Bug Description

Fix Data Race Risk in fiber/app.go
Description:
In the fiber/app.go file, there is a potential data race risk related to the app.server field. Specifically:
The app.server field is read/written 21 times.20 of these accesses are properly synchronized using app.mutex.RLock() or Lock().
1 read operation (in the Server() function at line 882) is not locked, which may cause a data race when Server() and other functions like init() are called concurrently.
Suggested Fix:
Use app.mutex.RLock() or Lock() consistently to protect all accesses to app.server.
And store the result in a local variable to minimize lock contention.
Could you please help confirm this bug?

How to Reproduce

Steps to reproduce the behavior:

Expected Behavior

Data Race

Fiber Version

v2.42.0

Code Snippet (optional)

Checklist:

• I agree to follow Fiber's Code of Conduct.
• I have checked for existing issues that describe my problem prior to opening this one.
• I understand that improperly formatted bug reports may be closed without explanation.

[welcome]

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

[ReneWerner87]

hi can you please give us more information and a test example

there is nothing in line 882

fiber/app.go

Line 882 in 42d921d

}

data races usually occur when you execute functions in parallel in threads, since you describe the server start here and this should not be executed in parallel, I am curious about the usecase

[ririli]

Hi, link to code app.go (v2.42.0):https://github.com/gofiber/fiber/blob/v2.42.0/app.go#L882
Here in line 882 you can see Server(), I found through static analysis that the server field is not mutex protected only here in line 882 in that code, while all others are mutex protected. This could have potential data race when Server() is called in parallel with other functions like init().

[gaby]

What software you used for static analysis?

[ReneWerner87]

does it make sense to include mutext there if no action is executed? mutext would have to be done outside if you want to make sure that no one customizes app.server

@coderabbitai how do other frameworks do this? what do you think as an independent AI?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check out the specific version mentioned in the issue
git checkout v2.42.0

# Look at the Server() function in app.go
echo "### Server() function:"
grep -n "func.*Server" app.go -A 5

# Look at all occurrences of app.server in the file
echo -e "\n### All app.server usages:"
grep -n "app.server" app.go

# Check mutex usage with server
echo -e "\n### Mutex usage with server:"
grep -n "mutex.*Lock.*server\|server.*mutex.*Lock" app.go -A 3

Length of output: 2074

---

🏁 Script executed:

#!/bin/bash
# Fetch the tags and check the correct version
git fetch --tags
git checkout v2.42.0

# First look at the Server() function in app.go 
echo "### Server() function and surrounding code:"
grep -n -A 5 -B 5 "func.*Server" app.go | grep -v "^\-\-$"

# Check all mutex usage around app.server
echo -e "\n### Mutex usage when accessing app.server:"
grep -n -A 5 -B 5 "app\.mutex.*Lock.*" app.go | grep -n "app\.server"
grep -n -A 5 -B 5 "app\.mutex.*RLock.*" app.go | grep -n "app\.server"

# Check how other operations access app.server
echo -e "\n### Other frameworks - let's check Echo as an example:"
git clone https://github.com/labstack/echo.git /tmp/echo
cd /tmp/echo
grep -r "mutex" --include="*.go" .

Length of output: 8314

---

@ReneWerner87 I've analyzed the code in question and can provide some insight about mutex protection for the Server() function.

Looking at the v2.42.0 code:

// Server returns the underlying fasthttp server
func (app *App) Server() *fasthttp.Server {
	return app.server
}

From a concurrency standpoint, it does make sense to protect this read with a mutex for several reasons:

1. Consistency principle: If 20 other places protect access to app.server with a mutex as mentioned in the issue, this suggests a design decision that the field should always be accessed under mutex protection.

2. Race condition risks: Even though this is a "read-only" operation, it can still be involved in a race condition if another goroutine is modifying app.server at the same time (perhaps during initialization or shutdown).

3. Visibility guarantees: In Go's memory model, a mutex not only prevents concurrent access but also ensures memory visibility across goroutines.

In other web frameworks like Echo, similar patterns are used where mutex protection is applied consistently. The general best practice is that if a field is accessed concurrently and protected by a mutex in some places, it should be protected in all places for consistency and correctness.

A simple fix would be:

func (app *App) Server() *fasthttp.Server {
	app.mutex.RLock()
	defer app.mutex.RUnlock()
	return app.server
}

This uses a read lock which allows concurrent reads but prevents reads during writes, providing thread safety with minimal performance impact.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the Server() method and surrounding code from app.go (v2.42.0)
curl -s https://raw.githubusercontent.com/gofiber/fiber/v2.42.0/app.go | grep -n -A10 -B2 "func (app \*App) Server()"

# Check other places where app.server is accessed with mutex locks
curl -s https://raw.githubusercontent.com/gofiber/fiber/v2.42.0/app.go | grep -n "app.server" | grep -v "func (app \*App) Server()"

# Check other places where mutex locks are used when accessing app.server
curl -s https://raw.githubusercontent.com/gofiber/fiber/v2.42.0/app.go | grep -n -A2 -B2 "app.mutex.R?Lock()" | grep -A2 -B2 "app.server"

Length of output: 2128

---

🏁 Script executed:

#!/bin/bash
# Check for mutex locks around app.server access
echo "Checking for mutex locks around app.server access:"
curl -s https://raw.githubusercontent.com/gofiber/fiber/v2.42.0/app.go | grep -A5 -B5 "app.mutex" | grep -B10 -A10 "app.server"

# Get the entire ShutdownWithContext method to see mutex usage
echo -e "\nShutdownWithContext method:"
curl -s https://raw.githubusercontent.com/gofiber/fiber/v2.42.0/app.go | grep -A20 -B2 "func (app \*App) ShutdownWithContext"

# Look at other Go web frameworks for comparison (Echo)
echo -e "\nChecking Echo framework's server access patterns:"
curl -s https://raw.githubusercontent.com/labstack/echo/master/echo.go | grep -A5 -B5 "Server"

Length of output: 8102

---

🏁 Script executed:

#!/bin/bash
# Get the entire Server() method from app.go in Fiber (v2.42.0)
echo "Server() method in Fiber:"
curl -s https://raw.githubusercontent.com/gofiber/fiber/v2.42.0/app.go | grep -A3 -B2 "func (app \*App) Server()"

# Check how the server is initialized and where it's potentially modified
echo -e "\nServer initialization in Fiber:"
curl -s https://raw.githubusercontent.com/gofiber/fiber/v2.42.0/app.go | grep -n -A5 "app.server = "

# Check for usage examples of Server() method
echo -e "\nLooking for usage of Server() method:"
curl -s https://api.github.com/search/code?q=Server+repo:gofiber/fiber+path:test | jq '.items[].path' | xargs -I{} echo "https://github.com/gofiber/fiber/blob/master/{}"

Length of output: 957

---

@ReneWerner87 I've examined how Fiber and other frameworks handle server access. Here's my analysis:

Looking at the code:

1. The Server() method simply returns app.server without any locking:

func (app *App) Server() *fasthttp.Server {
    return app.server
}

2. All other accesses to app.server are protected with mutex locks, including:
   • In shutdownWithContext: app.mutex.Lock() before checking and using app.server
   • During initialization: app.server = &fasthttp.Server{...} is wrapped with mutex locks

From a concurrency perspective, adding a read lock to the Server() method would be a good practice for several reasons:

1. Consistency: All other access points to app.server are protected with mutex locks. Being consistent avoids subtle bugs.

2. Race condition protection: If one goroutine is initializing or modifying app.server while another calls Server(), there's a potential race condition.

3. Standard Go pattern: It's common practice in Go to protect all shared state access with appropriate locks, even for read-only operations.

Similar frameworks like Echo also use mutex protection around server operations, though their approach is different (using a startupMutex that's locked during operations that modify the server).

I'd recommend adding a read lock to the Server() method:

func (app *App) Server() *fasthttp.Server {
    app.mutex.RLock()
    defer app.mutex.RUnlock()
    return app.server
}

This ensures thread safety with minimal performance impact since read locks allow concurrent reads.

[ReneWerner87]

ok, then lets do this

[gaby]

Doesnt that mean that Hooks() also needs this? Nevermind :-)

[ririli]

What software you used for static analysis?

Hello, it's our self-developed software based on the official go language tools package.

[ririli]

ok, then lets do this

Hi, excuse me, now how do you feel about this, do you think it needs a locking operation on it?