From 52d95f50729b40628ae77d667d262c5235e50cb8 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 7 Aug 2018 11:11:40 -0400 Subject: Propogate error codes from SHM lock creation and open Also add a few more unit tests Signed-off-by: Matthew Heon --- libpod/lock/locks.go | 14 ++++++------ libpod/lock/locks_test.go | 36 +++++++++++++++++++++++++++++++ libpod/lock/shm_lock.c | 55 ++++++++++++++++++++++++++++++++++++----------- libpod/lock/shm_lock.h | 4 ++-- 4 files changed, 88 insertions(+), 21 deletions(-) (limited to 'libpod') diff --git a/libpod/lock/locks.go b/libpod/lock/locks.go index 14da7d6a7..c61bd78d2 100644 --- a/libpod/lock/locks.go +++ b/libpod/lock/locks.go @@ -27,16 +27,17 @@ type SHMLocks struct { // semaphores, and returns a struct that can be used to operate on those locks. // numLocks must be a multiple of the lock bitmap size (by default, 32). func CreateSHMLock(numLocks uint32) (*SHMLocks, error) { - if numLocks % bitmapSize != 0 { + if numLocks % bitmapSize != 0 || numLocks == 0 { return nil, errors.Wrapf(syscall.EINVAL, "number of locks must be a multiple of %d", C.bitmap_size_c) } locks := new(SHMLocks) - lockStruct := C.setup_lock_shm(C.uint32_t(numLocks)) + var errCode C.int = 0 + lockStruct := C.setup_lock_shm(C.uint32_t(numLocks), &errCode) if lockStruct == nil { // We got a null pointer, so something errored - return nil, errors.Wrapf(syscall.ENOENT, "error creating shared memory locks") + return nil, syscall.Errno(-1 * errCode) } locks.lockStruct = lockStruct @@ -51,16 +52,17 @@ func CreateSHMLock(numLocks uint32) (*SHMLocks, error) { // segment was created with and be a multiple of the lock bitmap size (default // 32) func OpenSHMLock(numLocks uint32) (*SHMLocks, error) { - if numLocks % bitmapSize != 0 { + if numLocks % bitmapSize != 0 || numLocks == 0 { return nil, errors.Wrapf(syscall.EINVAL, "number of locks must be a multiple of %d", C.bitmap_size_c) } locks := new(SHMLocks) - lockStruct := C.open_lock_shm(C.uint32_t(numLocks)) + var errCode C.int = 0 + lockStruct := C.open_lock_shm(C.uint32_t(numLocks), &errCode) if lockStruct == nil { // We got a null pointer, so something errored - return nil, errors.Wrapf(syscall.ENOENT, "error creating shared memory locks") + return nil, syscall.Errno(-1 * errCode) } locks.lockStruct = lockStruct diff --git a/libpod/lock/locks_test.go b/libpod/lock/locks_test.go index ce4882643..62a36c5da 100644 --- a/libpod/lock/locks_test.go +++ b/libpod/lock/locks_test.go @@ -80,6 +80,42 @@ func TestCreateNewSHMBadSize(t *testing.T) { assert.Error(t, err) } +// Test that creating an SHM with 0 size fails +func TestCreateNewSHMZeroSize(t *testing.T) { + _, err := CreateSHMLock(0) + assert.Error(t, err) +} + +// Test that deallocating an unallocated lock errors +func TestDeallocateUnallocatedLockErrors(t *testing.T) { + runLockTest(t, func(t *testing.T, locks *SHMLocks) { + err := locks.DeallocateSemaphore(0) + assert.Error(t, err) + }) +} + +// Test that unlocking an unlocked lock fails +func TestUnlockingUnlockedLockFails(t *testing.T) { + runLockTest(t, func(t *testing.T, locks *SHMLocks) { + err := locks.UnlockSemaphore(0) + assert.Error(t, err) + }) +} + +// Test that locking and double-unlocking fails +func TestDoubleUnlockFails(t *testing.T) { + runLockTest(t, func(t *testing.T, locks *SHMLocks) { + err := locks.LockSemaphore(0) + assert.NoError(t, err) + + err = locks.UnlockSemaphore(0) + assert.NoError(t, err) + + err = locks.UnlockSemaphore(0) + assert.Error(t, err) + }) +} + // Test allocating - lock - unlock - deallocate cycle, single lock func TestLockLifecycleSingleLock(t *testing.T) { runLockTest(t, func(t *testing.T, locks *SHMLocks) { diff --git a/libpod/lock/shm_lock.c b/libpod/lock/shm_lock.c index 48fd4d4a9..3fe41f63c 100644 --- a/libpod/lock/shm_lock.c +++ b/libpod/lock/shm_lock.c @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -15,25 +16,32 @@ size_t compute_shm_size(uint32_t num_bitmaps) { return sizeof(shm_struct_t) + (num_bitmaps * sizeof(lock_group_t)); } -// Set up an SHM segment holding locks for libpod -// num_locks must be a multiple of BITMAP_SIZE (32 by default) -// Returns a valid pointer on success or NULL on error -shm_struct_t *setup_lock_shm(uint32_t num_locks) { +// Set up an SHM segment holding locks for libpod. +// num_locks must be a multiple of BITMAP_SIZE (32 by default). +// Returns a valid pointer on success or NULL on error. +// If an error occurs, it will be written to the int pointed to by error_code. +shm_struct_t *setup_lock_shm(uint32_t num_locks, int *error_code) { int shm_fd, i, j, ret_code; uint32_t num_bitmaps; size_t shm_size; shm_struct_t *shm; - // TODO maybe set errno so we can get errors back to libpod? + // If error_code doesn't point to anything, we can't reasonably return errors + // So fail immediately + if (error_code == NULL) { + return NULL; + } // We need a nonzero number of locks if (num_locks == 0) { + *error_code = EINVAL; return NULL; } // Calculate the number of bitmaps required if (num_locks % BITMAP_SIZE != 0) { // Number of locks not a multiple of BITMAP_SIZE + *error_code = EINVAL; return NULL; } num_bitmaps = num_locks / BITMAP_SIZE; @@ -44,18 +52,21 @@ shm_struct_t *setup_lock_shm(uint32_t num_locks) { // Create a new SHM segment for us shm_fd = shm_open(SHM_NAME, O_RDWR | O_CREAT | O_EXCL, 0600); if (shm_fd < 0) { + *error_code = errno; return NULL; } // Increase its size to what we need ret_code = ftruncate(shm_fd, shm_size); if (ret_code < 0) { + *error_code = errno; goto CLEANUP_UNLINK; } // Map the shared memory in shm = mmap(NULL, shm_size, PROT_READ | PROT_WRITE, MAP_SHARED, shm_fd, 0); if (shm == MAP_FAILED) { + *error_code = errno; goto CLEANUP_UNLINK; } @@ -64,9 +75,12 @@ shm_struct_t *setup_lock_shm(uint32_t num_locks) { shm->num_locks = num_locks; shm->num_bitmaps = num_bitmaps; - // Initialize the semaphore that protects the bitmaps - ret_code = sem_init(&(shm->segment_lock), 1, 1); + // Initialize the semaphore that protects the bitmaps. + // Initialize to value 1, as we're a mutex, and set pshared as this will be + // shared between processes in an SHM. + ret_code = sem_init(&(shm->segment_lock), true, 1); if (ret_code < 0) { + *error_code = errno; goto CLEANUP_UNMAP; } @@ -75,8 +89,11 @@ shm_struct_t *setup_lock_shm(uint32_t num_locks) { for (i = 0; i < num_bitmaps; i++) { shm->locks[i].bitmap = 0; for (j = 0; j < BITMAP_SIZE; j++) { - ret_code = sem_init(&(shm->locks[i].locks[j]), 1, 1); + // As above, initialize to 1 to act as a mutex, and set pshared as we'll + // be living in an SHM. + ret_code = sem_init(&(shm->locks[i].locks[j]), true, 1); if (ret_code < 0) { + *error_code = errno; goto CLEANUP_UNMAP; } } @@ -97,24 +114,31 @@ shm_struct_t *setup_lock_shm(uint32_t num_locks) { return NULL; } -// Open an existing SHM segment holding libpod locks -// num_locks is the number of locks that will be configured in the SHM segment -// num_locks must be a multiple of BITMAP_SIZE (32 by default) -// Returns a valid pointer on success or NULL on error -shm_struct_t *open_lock_shm(uint32_t num_locks) { +// Open an existing SHM segment holding libpod locks. +// num_locks is the number of locks that will be configured in the SHM segment. +// num_locks must be a multiple of BITMAP_SIZE (32 by default). +// Returns a valid pointer on success or NULL on error. +// If an error occurs, it will be written to the int pointed to by error_code. +shm_struct_t *open_lock_shm(uint32_t num_locks, int *error_code) { int shm_fd; shm_struct_t *shm; size_t shm_size; uint32_t num_bitmaps; + if (error_code == NULL) { + return NULL; + } + // We need a nonzero number of locks if (num_locks == 0) { + *error_code = EINVAL; return NULL; } // Calculate the number of bitmaps required if (num_locks % BITMAP_SIZE != 0) { // Number of locks not a multiple of BITMAP_SIZE + *error_code = EINVAL; return NULL; } num_bitmaps = num_locks / BITMAP_SIZE; @@ -129,6 +153,9 @@ shm_struct_t *open_lock_shm(uint32_t num_locks) { // Map the shared memory in shm = mmap(NULL, shm_size, PROT_READ | PROT_WRITE, MAP_SHARED, shm_fd, 0); + if (shm == MAP_FAILED) { + *error_code = errno; + } // Ignore errors, it's ok if we leak a single FD since this only runs once close(shm_fd); @@ -140,9 +167,11 @@ shm_struct_t *open_lock_shm(uint32_t num_locks) { // Need to check the SHM to see if it's actually our locks if (shm->magic != MAGIC) { + *error_code = errno; goto CLEANUP; } if (shm->num_locks != num_locks) { + *error_code = errno; goto CLEANUP; } diff --git a/libpod/lock/shm_lock.h b/libpod/lock/shm_lock.h index 6988789dd..18bea47e9 100644 --- a/libpod/lock/shm_lock.h +++ b/libpod/lock/shm_lock.h @@ -32,8 +32,8 @@ typedef struct shm_struct { } shm_struct_t; size_t compute_shm_size(uint32_t num_bitmaps); -shm_struct_t *setup_lock_shm(uint32_t num_locks); -shm_struct_t *open_lock_shm(uint32_t num_locks); +shm_struct_t *setup_lock_shm(uint32_t num_locks, int *error_code); +shm_struct_t *open_lock_shm(uint32_t num_locks, int *error_code); int32_t close_lock_shm(shm_struct_t *shm); int64_t allocate_semaphore(shm_struct_t *shm); int32_t deallocate_semaphore(shm_struct_t *shm, uint32_t sem_index); -- cgit v1.2.3-54-g00ecf