[PATCH 0/1] MR3040: services: Fix use after free on error path in service_start (scan-build).

[PATCH 1/1] services: Fix use after free on error path in service_start (scan-build).

[Alex Henrie]

From: Alex Henrie

---
 programs/services/services.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/programs/services/services.c b/programs/services/services.c
index 2db74f99e0f..a77c17c58a9 100644
--- a/programs/services/services.c
+++ b/programs/services/services.c
@@ -1214,7 +1214,6 @@ DWORD service_start(struct service_entry *service, DWORD service_argc, LPCWSTR *
                 service->status.dwCurrentState = SERVICE_STOPPED;
                 service->process = NULL;
                 if (!--process->use_count) process_terminate(process);
-                release_process(process);
             }
             service_unlock(service);
         }

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about programs/services/services.c:
>              service->status.dwCurrentState = SERVICE_STOPPED;
>              service->process = NULL;
>              if (!--process->use_count) process_terminate(process);
> -
>            release_process(process);

I don't see use after free error here. process_entry reference is stored in 2 places - in process variable and in service->process. Since The code is setting service->process to NULL it needs to decrease the reference counter. Later we're freeing process variable so another release_process call is needed.

I guess the tool doesn't know that process == service->process.

[Alex Henrie (＠alexhenrie)]

On Mon Jun 12 12:02:42 2023 +0000, Piotr Caban wrote:
> I don't see use after free error here. process_entry reference is stored
> in 2 places - in process variable and in service->process. Since The
> code is setting service->process to NULL it needs to decrease the
> reference counter. Later we're freeing process variable so another
> release_process call is needed.
> I guess the tool doesn't know that process == service->process.

scan-build thought that calling `release_process(process)` would free `process` before it is dereferenced in `ReleaseMutex(process->control_mutex)` a few lines later. But you're right, it's ref-counted, so there's no real problem here. Sorry for the noise.

[PATCH 0/1] MR3040: services: Fix use after free on error path in service_start (scan-build). - closed

[Alex Henrie (＠alexhenrie)]

This merge request was closed by Alex Henrie.