Merge pull request #1535 from LinuxCNC/hal-init-race

fix hal_init() race

Author: SebKuzminsky

docs/man/man3/rtapi_shmem.3rtapi

@@ -35,8 +35,8 @@ wishing to access the same memory must use the same key.
 \fImodule_id\fR is the ID of the module that is making the call (see
 rtapi_init).  The block will be at least \fIsize\fR bytes, and may
 be rounded up.  Allocating many small blocks may be very wasteful.
-When a particular block is allocated for the first time, the first
-4 bytes are zeroed.  Subsequent allocations of the same block
+When a particular block is allocated for the first time, the contents
+are zeroed.  Subsequent allocations of the same block
 by other modules or processes will not touch the contents of the
 block.  Applications can use those bytes to see if they need to 
 initialize the block, or if another module already did so.

src/hal/hal_lib.c

@@ -349,7 +349,7 @@ int hal_exit(int comp_id)
     --ref_cnt;
 #ifdef ULAPI
     if(ref_cnt == 0) {
-        rtapi_print_msg(RTAPI_MSG_DBG, "HAL:  releasing RTAPI resources");
+        rtapi_print_msg(RTAPI_MSG_DBG, "HAL: releasing RTAPI resources\n");
 	/* release RTAPI resources */
 	rtapi_shmem_delete(lib_mem_id, lib_module_id);
 	rtapi_exit(lib_module_id);
@@ -2781,19 +2781,29 @@ static void thread_task(void *arg)
 
 static int init_hal_data(void)
 {
-    /* has the block already been initialized? */
+    /* has the hal_data block already been initialized? */
+
+    /* Lock hal_data by taking the mutex, so that two processes
+    don't both try to initialize hal_data at the same time.  NOTE:
+    The first time through, the hal_data memory buffer is fresh from
+    rtapi_shmem_new(), which means it's initialized to all zero bytes.
+    This means hal_data->mutex is valid and unlocked. */
+    rtapi_mutex_get(&(hal_data->mutex));
+
     if (hal_data->version != 0) {
-	/* yes, verify version code */
-	if (hal_data->version == HAL_VER) {
-	    return 0;
-	} else {
-	    rtapi_print_msg(RTAPI_MSG_ERR,
-		"HAL: ERROR: version code mismatch\n");
-	    return -1;
-	}
+        /* hal_data has been initialized already, verify version code */
+        if (hal_data->version == HAL_VER) {
+            rtapi_mutex_give(&(hal_data->mutex));
+            return 0;
+        } else {
+            rtapi_print_msg(RTAPI_MSG_ERR, "HAL: ERROR: version code mismatch\n");
+            rtapi_mutex_give(&(hal_data->mutex));
+            return -1;
+        }
     }
-    /* no, we need to init it, grab the mutex unconditionally */
-    rtapi_mutex_try(&(hal_data->mutex));
+
+    /* hal_data has *NOT* been initialized yet, we get the honor */
+
     /* set version code so nobody else init's the block */
     hal_data->version = HAL_VER;
     /* initialize everything */

src/rtapi/rtapi.h

@@ -431,11 +431,10 @@ RTAPI_BEGIN_DECLS
     'module_id' is the ID of the module that is making the call (see
     rtapi_init).  The block will be at least 'size' bytes, and may
     be rounded up.  Allocating many small blocks may be very wasteful.
-    When a particular block is allocated for the first time, the first
-    4 bytes are zeroed.  Subsequent allocations of the same block
+    When a particular block is allocated for the first time, all
+    bytes are zeroed.  Subsequent allocations of the same block
     by other modules or processes will not touch the contents of the
-    block.  Applications can use those bytes to see if they need to
-    initialize the block, or if another module already did so.
+    block.
     On success, it returns a positive integer ID, which is used for
     all subsequent calls dealing with the block.  On failure it
     returns a negative error code.  Call only from within user or

tests/rtapi-shmem/README.md

@@ -0,0 +1,2 @@
+This test allocates some shared memory from RTAPI, and verifies that it
+is initialized to all bytes zero.

tests/rtapi-shmem/checkresult

@@ -0,0 +1,2 @@
+#!/bin/bash
+exit 0

tests/rtapi-shmem/setup.hal

@@ -0,0 +1,2 @@
+debug 5
+loadrt test_shmem_rtcomp

tests/rtapi-shmem/test.sh

@@ -0,0 +1,5 @@
+#!/bin/sh
+
+halcompile --install test_shmem_rtcomp.comp
+
+halrun -V setup.hal

tests/rtapi-shmem/test_shmem_rtcomp.comp

@@ -0,0 +1,119 @@
+//  Copyright 2022 Sebastian Kuzminsky <seb@highlab.com>
+//
+//  This program is free software; you can redistribute it and/or
+//  modify it under the terms of version 2 of the GNU General
+//  Public License as published by the Free Software Foundation.
+//
+//  This program is distributed in the hope that it will be useful,
+//  but WITHOUT ANY WARRANTY; without even the implied warranty of
+//  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+//  GNU General Public License for more details.
+//
+//  You should have received a copy of the GNU General Public License
+//  along with this program; if not, write to the Free Software
+//  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+component test_shmem_rtcomp "Validate rtapi's shmem initialization behavior";
+
+option extra_setup;
+option extra_cleanup;
+option singleton;
+
+pin out bit ready = 0 "Goes true when shmem is initialized";
+
+function _;
+license "GPL";
+
+;;
+
+#include <rtapi.h>
+
+#define SHMEM_KEY 9999
+#define SHMEM_SIZE (4096 * 100) // in bytes
+
+int shmem_id = -1;
+void *mem = NULL;
+
+int shmem_alloc(void) {
+    int i;
+    int r;
+    uint8_t *p;
+
+    rtapi_print_msg(RTAPI_MSG_ERR, "allocating shmem\n");
+
+    shmem_id = rtapi_shmem_new(SHMEM_KEY, comp_id, SHMEM_SIZE);
+    if (shmem_id < 0) {
+        rtapi_print_msg(RTAPI_MSG_ERR, "failed to make new rtapi shmem\n");
+        return -EINVAL;
+    }
+
+    r = rtapi_shmem_getptr(shmem_id, &mem);
+    if (r < 0) {
+        rtapi_print_msg(RTAPI_MSG_ERR, "failed to get new rtapi shmem\n");
+        return -EINVAL;
+    }
+    if (mem == NULL) {
+        rtapi_print_msg(RTAPI_MSG_ERR, "new rtapi shmem is NULL??\n");
+        return -EINVAL;
+    }
+
+    p = mem;
+    for (i = 0; i < SHMEM_SIZE; ++i) {
+        if (p[i] != 0x00) {
+            rtapi_print_msg(RTAPI_MSG_ERR, "shmem is not initialized to 0x00!\n");
+            return -EINVAL;
+        }
+    }
+
+    rtapi_print_msg(RTAPI_MSG_ERR, "allocated shmem came initialized to all bytes zero\n");
+
+    return 0;
+}
+
+
+void shmem_free(void) {
+    if (shmem_id >= 0) {
+        rtapi_print_msg(RTAPI_MSG_ERR, "freeing shmem\n");
+        rtapi_shmem_delete(shmem_id, comp_id);
+    }
+}
+
+
+EXTRA_SETUP() {
+    int i;
+    int r;
+    uint8_t *p;
+
+    r = shmem_alloc();
+    if (r < 0) {
+        rtapi_print_msg(RTAPI_MSG_ERR, "failed to alloc shmem\n");
+        return -EINVAL;
+    }
+
+    // dirty the shmem
+    p = mem;
+    for (i = 0; i < SHMEM_SIZE; ++i) {
+        p[i] = i % 256;
+    }
+
+    shmem_free();
+
+    r = shmem_alloc();
+    if (r < 0) {
+        rtapi_print_msg(RTAPI_MSG_ERR, "failed to alloc shmem\n");
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+
+EXTRA_CLEANUP() {
+    shmem_free();
+}
+
+
+FUNCTION(_) {
+    ready = 1;
+}
+