On 06/28/2010 04:43 PM, Angus Salkeld wrote:
> Steve, Not exactly the same as your latest patch ...
>
> - use snprintf()
> - use write to test that we can actually use all of the file.
>
> Signed-off-by: Angus Salkeld<asalk...@redhat.com>
> ---
> exec/coroipcs.c | 74 +++++++++++++++++++++++++++++++++++-----------
> lib/coroipcc.c | 88
> ++++++++++++++++++++++++++++++++++++++++---------------
> 2 files changed, 120 insertions(+), 42 deletions(-)
>
> diff --git a/exec/coroipcs.c b/exec/coroipcs.c
> index a2deb8a..1a084d5 100644
> --- a/exec/coroipcs.c
> +++ b/exec/coroipcs.c
> @@ -254,10 +254,13 @@ memory_map (
> size_t bytes,
> void **buf)
> {
> - int fd;
> + int32_t fd;
> void *addr_orig;
> void *addr;
> - int res;
> + int32_t res;
> + char buffer[128];
> + int32_t i;
> + int32_t written;
>
> fd = open (path, O_RDWR, 0600);
>
> @@ -269,24 +272,33 @@ memory_map (
>
> res = ftruncate (fd, bytes);
> if (res == -1) {
> - close (fd);
> - return (-1);
> + goto error_close_unlink;
> + }
> + memset (buffer, 0, sizeof (buffer));
> + for (i = 0; i< (bytes / 64); i++) {
> +retry_write:
> + written = write (fd, buffer, 64);
> + if (written == -1&& errno == EINTR) {
> + goto retry_write;
> + }
> + if (written != 64) {
> + goto error_close_unlink;
> + }
> }
>
> addr_orig = mmap (NULL, bytes, PROT_NONE,
> MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>
> if (addr_orig == MAP_FAILED) {
> - close (fd);
> - return (-1);
> + goto error_close_unlink;
> }
>
> addr = mmap (addr_orig, bytes, PROT_READ | PROT_WRITE,
> MAP_FIXED | MAP_SHARED, fd, 0);
>
> if (addr != addr_orig) {
> - close (fd);
> - return (-1);
> + munmap(addr_orig, bytes);
> + goto error_close_unlink;
> }
> #ifdef COROSYNC_BSD
> madvise(addr, bytes, MADV_NOSYNC);
> @@ -298,6 +310,11 @@ memory_map (
> }
> *buf = addr_orig;
> return (0);
> +
> +error_close_unlink:
> + close (fd);
> + unlink(path);
> + return -1;
> }
>
> static int
> @@ -306,10 +323,13 @@ circular_memory_map (
> size_t bytes,
> void **buf)
> {
> - int fd;
> + int32_t fd;
> void *addr_orig;
> void *addr;
> - int res;
> + int32_t res;
> + char buffer[128];
> + int32_t i;
> + int32_t written;
>
> fd = open (path, O_RDWR, 0600);
>
> @@ -320,24 +340,34 @@ circular_memory_map (
> }
> res = ftruncate (fd, bytes);
> if (res == -1) {
> - close (fd);
> - return (-1);
> + goto error_close_unlink;
> + }
> + memset (buffer, 0, sizeof (buffer));
> + for (i = 0; i< (bytes / 64); i++) {
> +retry_write:
> + written = write (fd, buffer, 64);
> + if (written == -1&& errno == EINTR) {
> + goto retry_write;
> + }
> + if (written != 64) {
> + goto error_close_unlink;
> + }
> }
>
> addr_orig = mmap (NULL, bytes<< 1, PROT_NONE,
> MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>
> if (addr_orig == MAP_FAILED) {
> - close (fd);
> - return (-1);
> + munmap(addr_orig, bytes);
> + goto error_close_unlink;
> }
>
> addr = mmap (addr_orig, bytes, PROT_READ | PROT_WRITE,
> MAP_FIXED | MAP_SHARED, fd, 0);
>
> if (addr != addr_orig) {
> - close (fd);
> - return (-1);
> + munmap(addr_orig, bytes);
> + goto error_close_unlink;
> }
> #ifdef COROSYNC_BSD
> madvise(addr_orig, bytes, MADV_NOSYNC);
> @@ -347,8 +377,9 @@ circular_memory_map (
> bytes, PROT_READ | PROT_WRITE,
> MAP_FIXED | MAP_SHARED, fd, 0);
> if (addr == MAP_FAILED) {
> - close (fd);
> - return (-1);
> + munmap(addr_orig, bytes);
> + munmap(addr, bytes);
> + goto error_close_unlink;
> }
> #ifdef COROSYNC_BSD
> madvise(((char *)addr_orig) + bytes, bytes, MADV_NOSYNC);
> @@ -356,10 +387,17 @@ circular_memory_map (
>
> res = close (fd);
> if (res) {
> + munmap(addr_orig, bytes);
> + munmap(addr, bytes);
> return (-1);
> }
> *buf = addr_orig;
> return (0);
> +
> +error_close_unlink:
> + close (fd);
> + unlink(path);
> + return (-1);
> }
>
> static inline int
> diff --git a/lib/coroipcc.c b/lib/coroipcc.c
> index 0d2d13b..944a85c 100644
> --- a/lib/coroipcc.c
> +++ b/lib/coroipcc.c
> @@ -40,6 +40,7 @@
> #include<stdlib.h>
> #include<stdio.h>
> #include<unistd.h>
> +#include<limits.h>
> #include<errno.h>
> #include<string.h>
> #include<fcntl.h>
> @@ -283,16 +284,19 @@ union semun {
> static int
> circular_memory_map (char *path, const char *file, void **buf, size_t bytes)
> {
> - int fd;
> + int32_t fd;
> void *addr_orig;
> void *addr;
> - int res;
> + int32_t res;
> + char buffer[128];
> + int32_t i;
> + int32_t written;
>
> - sprintf (path, "/dev/shm/%s", file);
> + snprintf (path, PATH_MAX, "/dev/shm/%s", file);
>
> fd = mkstemp (path);
> if (fd == -1) {
> - sprintf (path, LOCALSTATEDIR "/run/%s", file);
> + snprintf (path, PATH_MAX, LOCALSTATEDIR "/run/%s", file);
> fd = mkstemp (path);
> if (fd == -1) {
> return (-1);
> @@ -301,24 +305,33 @@ circular_memory_map (char *path, const char *file, void
> **buf, size_t bytes)
>
> res = ftruncate (fd, bytes);
> if (res == -1) {
> - close (fd);
> - return (-1);
> + goto error_close_unlink;
> + }
> + memset (buffer, 0, sizeof (buffer));
> + for (i = 0; i< (bytes / 64); i++) {
> +retry_write:
> + written = write (fd, buffer, 64);
> + if (written == -1&& errno == EINTR) {
> + goto retry_write;
> + }
> + if (written != 64) {
> + goto error_close_unlink;
> + }
> }
>
> addr_orig = mmap (NULL, bytes<< 1, PROT_NONE,
> MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>
> if (addr_orig == MAP_FAILED) {
> - close (fd);
> - return (-1);
> + goto error_close_unlink;
> }
>
> addr = mmap (addr_orig, bytes, PROT_READ | PROT_WRITE,
> MAP_FIXED | MAP_SHARED, fd, 0);
>
> if (addr != addr_orig) {
> - close (fd);
> - return (-1);
> + munmap(addr_orig, bytes);
> + goto error_close_unlink;
> }
> #ifdef COROSYNC_BSD
> madvise(addr_orig, bytes, MADV_NOSYNC);
> @@ -328,8 +341,9 @@ circular_memory_map (char *path, const char *file, void
> **buf, size_t bytes)
> bytes, PROT_READ | PROT_WRITE,
> MAP_FIXED | MAP_SHARED, fd, 0);
> if (addr == MAP_FAILED) {
> - close (fd);
> - return (-1);
> + munmap(addr_orig, bytes);
> + munmap(addr, bytes);
> + goto error_close_unlink;
> }
> #ifdef COROSYNC_BSD
> madvise(((char *)addr_orig) + bytes, bytes, MADV_NOSYNC);
> @@ -337,10 +351,17 @@ circular_memory_map (char *path, const char *file, void
> **buf, size_t bytes)
>
> res = close (fd);
> if (res) {
> + munmap(addr_orig, bytes);
> + munmap(addr, bytes);
> return (-1);
> }
> *buf = addr_orig;
> return (0);
> +
> +error_close_unlink:
> + close (fd);
> + unlink(path);
> + return (-1);
> }
>
> static void
> @@ -362,19 +383,23 @@ void ipc_hdb_destructor (void *context ) {
> memory_unmap (ipc_instance->response_buffer,
> ipc_instance->response_size);
> memory_unmap (ipc_instance->dispatch_buffer,
> (ipc_instance->dispatch_size)<< 1);
> }
> +
> static int
> memory_map (char *path, const char *file, void **buf, size_t bytes)
> {
> - int fd;
> + int32_t fd;
> void *addr_orig;
> void *addr;
> - int res;
> + int32_t res;
> + char buffer[128];
> + int32_t i;
> + int32_t written;
>
> - sprintf (path, "/dev/shm/%s", file);
> + snprintf (path, PATH_MAX, "/dev/shm/%s", file);
>
> fd = mkstemp (path);
> if (fd == -1) {
> - sprintf (path, LOCALSTATEDIR "/run/%s", file);
> + snprintf (path, PATH_MAX, LOCALSTATEDIR "/run/%s", file);
> fd = mkstemp (path);
> if (fd == -1) {
> return (-1);
> @@ -383,24 +408,33 @@ memory_map (char *path, const char *file, void **buf,
> size_t bytes)
>
> res = ftruncate (fd, bytes);
> if (res == -1) {
> - close (fd);
> - return (-1);
> + goto error_close_unlink;
> + }
> + memset (buffer, 0, sizeof (buffer));
> + for (i = 0; i< (bytes / 64); i++) {
> +retry_write:
> + written = write (fd, buffer, 64);
> + if (written == -1&& errno == EINTR) {
> + goto retry_write;
> + }
> + if (written != 64) {
> + goto error_close_unlink;
> + }
> }
>
> addr_orig = mmap (NULL, bytes, PROT_NONE,
> MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>
> if (addr_orig == MAP_FAILED) {
> - close (fd);
> - return (-1);
> + goto error_close_unlink;
> }
>
> addr = mmap (addr_orig, bytes, PROT_READ | PROT_WRITE,
> MAP_FIXED | MAP_SHARED, fd, 0);
>
> if (addr != addr_orig) {
> - close (fd);
> - return (-1);
> + munmap(addr_orig, bytes);
> + goto error_close_unlink;
> }
> #ifdef COROSYNC_BSD
> madvise(addr_orig, bytes, MADV_NOSYNC);
> @@ -411,7 +445,13 @@ memory_map (char *path, const char *file, void **buf,
> size_t bytes)
> return (-1);
> }
> *buf = addr_orig;
> - return (0);
> +
> + return 0;
> +
> +error_close_unlink:
> + close (fd);
> + unlink(path);
> + return -1;
> }
>
> static cs_error_t
> @@ -479,7 +519,7 @@ ipc_sem_wait (
> #else
> struct timespec timeout;
> struct pollfd pfd;
> - sem_t *sem;
> + sem_t *sem = NULL;
> #endif
> int res;
>
the fd should be closed when the mmap operation is done. This will
release the file descriptor from the process descriptor in the kernel
once munmap is executed. Otherwise this leaks file descriptors.
A partial map will result in leakage ie:
sys_res = memory_map (
response_map_path,
"response_buffer-XXXXXX",
(void *)&ipc_instance->response_buffer,
response_size);
if (sys_res == -1) {
res = CS_ERR_LIBRARY;
goto error_response_buffer;
}
sys_res = circular_memory_map (
dispatch_map_path,
"dispatch_buffer-XXXXXX",
(void *)&ipc_instance->dispatch_buffer,
dispatch_size);
if (sys_res == -1) {
res = CS_ERR_LIBRARY;
goto error_dispatch_buffer;
}
if the dispatch fails, the response buffer will be leaked.
Regards
-steve
_______________________________________________
Openais mailing list
Openais@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/openais
