From 2c5de8ed262aa0fcf1d1472e963d2009f793e047 Mon Sep 17 00:00:00 2001 From: Tor Lillqvist Date: Wed, 27 Aug 2008 16:49:17 +0000 Subject: [PATCH] Stylistic changes. Plug an unlikely memory leak that occurred in 2008-08-27 Tor Lillqvist * glib/giowin32.c: Stylistic changes. Plug an unlikely memory leak that occurred in create_thread() if closing the thread handle failed. Add more error messages to g_io_win32_free() that are printed only when debugging. Plug handle leak, a socket channel's event was never closed. svn path=/trunk/; revision=7405 --- ChangeLog | 8 ++++ glib/giowin32.c | 143 ++++++++++++++++++++++++++++++++++++++------------------ 2 files changed, 105 insertions(+), 46 deletions(-) diff --git a/ChangeLog b/ChangeLog index a996c46..17e779a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,13 @@ 2008-08-27 Tor Lillqvist + * glib/giowin32.c: Stylistic changes. Plug an unlikely memory leak + that occurred in create_thread() if closing the thread handle + failed. Add more error messages to g_io_win32_free() that are + printed only when debugging. Plug handle leak, a socket channel's + event was never closed. + +2008-08-27 Tor Lillqvist + * config.h.win32.in: Should not define HAVE_DIRENT_H when compiling with MSVC, as the only file which checks HAVE_DIRENT_H is gdir.c, and that includes the dirent.h and wdirent.c from diff --git a/glib/giowin32.c b/glib/giowin32.c index aba51ec..c31aa8e 100644 --- a/glib/giowin32.c +++ b/glib/giowin32.c @@ -163,9 +163,6 @@ struct _GIOWin32Channel { gboolean ever_writable; }; -#define LOCK(mutex) EnterCriticalSection (&mutex) -#define UNLOCK(mutex) LeaveCriticalSection (&mutex) - struct _GIOWin32Watch { GSource source; GPollFD pollfd; @@ -276,19 +273,21 @@ static void g_io_channel_win32_init (GIOWin32Channel *channel) { channel->debug = g_io_win32_get_debug_flag (); - channel->buffer = NULL; + + InitializeCriticalSection (&channel->mutex); channel->running = FALSE; channel->needs_close = FALSE; channel->thread_id = 0; channel->data_avail_event = NULL; channel->revents = 0; + channel->buffer = NULL; channel->space_avail_event = NULL; + channel->event_mask = 0; channel->last_events = 0; channel->event = NULL; channel->write_would_have_blocked = FALSE; channel->ever_writable = FALSE; - InitializeCriticalSection (&channel->mutex); } static void @@ -307,6 +306,7 @@ create_events (GIOWin32Channel *channel) || !(channel->space_avail_event = CreateEvent (&sec_attrs, FALSE, FALSE, NULL))) { gchar *emsg = g_win32_error_message (GetLastError ()); + g_error ("Error creating event: %s", emsg); g_free (emsg); } @@ -335,7 +335,7 @@ read_thread (void *parameter) SetEvent (channel->space_avail_event); - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); while (channel->running) { if (channel->debug) @@ -351,9 +351,9 @@ read_thread (void *parameter) if (channel->debug) g_print ("read_thread %#x: waiting for space\n", channel->thread_id); - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); WaitForSingleObject (channel->space_avail_event, INFINITE); - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); if (channel->debug) g_print ("read_thread %#x: rdp=%d, wrp=%d\n", channel->thread_id, channel->rdp, channel->wrp); @@ -371,11 +371,11 @@ read_thread (void *parameter) g_print ("read_thread %#x: calling read() for %d bytes\n", channel->thread_id, nbytes); - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); nbytes = read (channel->fd, buffer, nbytes); - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); channel->revents = G_IO_IN; if (nbytes == 0) @@ -411,7 +411,7 @@ read_thread (void *parameter) g_print ("read_thread %#x: EOF, rdp=%d, wrp=%d, setting data_avail\n", channel->thread_id, channel->rdp, channel->wrp); SetEvent (channel->data_avail_event); - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); g_io_channel_unref ((GIOChannel *)channel); @@ -452,7 +452,7 @@ write_thread (void *parameter) * write buffer. */ - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); while (channel->running || channel->rdp != channel->wrp) { if (channel->debug) @@ -470,10 +470,10 @@ write_thread (void *parameter) channel->thread_id); channel->revents = G_IO_OUT; SetEvent (channel->data_avail_event); - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); WaitForSingleObject (channel->space_avail_event, INFINITE); - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); if (channel->rdp == channel->wrp) break; @@ -492,9 +492,9 @@ write_thread (void *parameter) g_print ("write_thread %#x: calling write() for %d bytes\n", channel->thread_id, nbytes); - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); nbytes = write (channel->fd, buffer, nbytes); - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); if (channel->debug) g_print ("write_thread %#x: write(%i) returned %d, rdp=%d, wrp=%d\n", @@ -527,7 +527,7 @@ write_thread (void *parameter) channel->fd = -1; } - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); g_io_channel_unref ((GIOChannel *)channel); @@ -547,8 +547,12 @@ create_thread (GIOWin32Channel *channel, g_warning ("Error creating thread: %s.", g_strerror (errno)); else if (!CloseHandle (thread_handle)) - g_warning ("Error closing thread handle: %s.\n", - g_win32_error_message (GetLastError ())); + { + gchar *emsg = g_win32_error_message (GetLastError ()); + + g_warning ("Error closing thread handle: %s.", emsg); + g_free (emsg); + } WaitForSingleObject (channel->space_avail_event, INFINITE); } @@ -563,25 +567,25 @@ buffer_read (GIOWin32Channel *channel, guint nbytes; guint left = count; - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); if (channel->debug) g_print ("reading from thread %#x %" G_GSIZE_FORMAT " bytes, rdp=%d, wrp=%d\n", channel->thread_id, count, channel->rdp, channel->wrp); if (channel->wrp == channel->rdp) { - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); if (channel->debug) g_print ("waiting for data from thread %#x\n", channel->thread_id); WaitForSingleObject (channel->data_avail_event, INFINITE); if (channel->debug) g_print ("done waiting for data from thread %#x\n", channel->thread_id); - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); if (channel->wrp == channel->rdp && !channel->running) { if (channel->debug) g_print ("wrp==rdp, !running\n"); - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); *bytes_read = 0; return G_IO_STATUS_EOF; } @@ -591,7 +595,7 @@ buffer_read (GIOWin32Channel *channel, nbytes = channel->wrp - channel->rdp; else nbytes = BUFFER_SIZE - channel->rdp; - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); nbytes = MIN (left, nbytes); if (channel->debug) g_print ("moving %d bytes from thread %#x\n", @@ -599,7 +603,7 @@ buffer_read (GIOWin32Channel *channel, memcpy (dest, channel->buffer + channel->rdp, nbytes); dest += nbytes; left -= nbytes; - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); channel->rdp = (channel->rdp + nbytes) % BUFFER_SIZE; if (channel->debug) g_print ("setting space_avail for thread %#x\n", channel->thread_id); @@ -614,7 +618,7 @@ buffer_read (GIOWin32Channel *channel, channel->thread_id); ResetEvent (channel->data_avail_event); }; - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); /* We have no way to indicate any errors form the actual * read() or recv() call in the reader thread. Should we have? @@ -634,7 +638,7 @@ buffer_write (GIOWin32Channel *channel, guint nbytes; guint left = count; - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); if (channel->debug) g_print ("buffer_write: writing to thread %#x %" G_GSIZE_FORMAT " bytes, rdp=%d, wrp=%d\n", channel->thread_id, count, channel->rdp, channel->wrp); @@ -649,9 +653,9 @@ buffer_write (GIOWin32Channel *channel, if (channel->debug) g_print ("buffer_write: tid %#x: waiting for space\n", channel->thread_id); - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); WaitForSingleObject (channel->data_avail_event, INFINITE); - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); if (channel->debug) g_print ("buffer_write: tid %#x: rdp=%d, wrp=%d\n", channel->thread_id, channel->rdp, channel->wrp); @@ -660,7 +664,7 @@ buffer_write (GIOWin32Channel *channel, nbytes = MIN ((channel->rdp + BUFFER_SIZE - channel->wrp - 1) % BUFFER_SIZE, BUFFER_SIZE - channel->wrp); - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); nbytes = MIN (left, nbytes); if (channel->debug) g_print ("buffer_write: tid %#x: writing %d bytes\n", @@ -668,7 +672,7 @@ buffer_write (GIOWin32Channel *channel, memcpy (channel->buffer + channel->wrp, dest, nbytes); dest += nbytes; left -= nbytes; - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); channel->wrp = (channel->wrp + nbytes) % BUFFER_SIZE; if (channel->debug) @@ -685,7 +689,7 @@ buffer_write (GIOWin32Channel *channel, ResetEvent (channel->data_avail_event); } - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); /* We have no way to indicate any errors form the actual * write() call in the writer thread. Should we have? @@ -730,7 +734,7 @@ g_io_win32_prepare (GSource *source, condition_to_string (watch->pollfd.revents), condition_to_string (channel->revents)); - LOCK (channel->mutex); + EnterCriticalSection (&channel->mutex); if (channel->running) { if (channel->direction == 0 && channel->wrp == channel->rdp) @@ -750,7 +754,7 @@ g_io_win32_prepare (GSource *source, channel->revents = 0; } } - UNLOCK (channel->mutex); + LeaveCriticalSection (&channel->mutex); break; case G_IO_WIN32_SOCKET: @@ -771,7 +775,13 @@ g_io_win32_prepare (GSource *source, event_mask_to_string (event_mask)); if (WSAEventSelect (channel->fd, (HANDLE) watch->pollfd.fd, event_mask) == SOCKET_ERROR) - ; /* What? */ + if (channel->debug) + { + gchar *emsg = g_win32_error_message (WSAGetLastError ()); + + g_print (" failed: %s", emsg); + g_free (emsg); + } channel->event_mask = event_mask; if (channel->debug) @@ -1068,8 +1078,10 @@ g_io_win32_msg_write (GIOChannel *channel, if (!PostMessage (win32_channel->hwnd, msg.message, msg.wParam, msg.lParam)) { gchar *emsg = g_win32_error_message (GetLastError ()); + g_set_error_literal (err, G_IO_CHANNEL_ERROR, G_IO_CHANNEL_ERROR_FAILED, emsg); g_free (emsg); + return G_IO_STATUS_ERROR; } @@ -1095,15 +1107,54 @@ g_io_win32_free (GIOChannel *channel) if (win32_channel->debug) g_print ("g_io_win32_free channel=%p fd=%d\n", channel, win32_channel->fd); + DeleteCriticalSection (&win32_channel->mutex); + if (win32_channel->data_avail_event) - CloseHandle (win32_channel->data_avail_event); + if (!CloseHandle (win32_channel->data_avail_event)) + if (win32_channel->debug) + { + gchar *emsg = g_win32_error_message (GetLastError ()); + + g_print (" CloseHandle(%p) failed: %s\n", + win32_channel->data_avail_event, emsg); + g_free (emsg); + } + + g_free (win32_channel->buffer); + if (win32_channel->space_avail_event) - CloseHandle (win32_channel->space_avail_event); + if (!CloseHandle (win32_channel->space_avail_event)) + if (win32_channel->debug) + { + gchar *emsg = g_win32_error_message (GetLastError ()); + + g_print (" CloseHandle(%p) failed: %s\n", + win32_channel->space_avail_event, emsg); + g_free (emsg); + } + if (win32_channel->type == G_IO_WIN32_SOCKET) - WSAEventSelect (win32_channel->fd, NULL, 0); - DeleteCriticalSection (&win32_channel->mutex); + if (WSAEventSelect (win32_channel->fd, NULL, 0) == SOCKET_ERROR) + if (win32_channel->debug) + { + gchar *emsg = g_win32_error_message (WSAGetLastError ()); + + g_print (" WSAEventSelect(%d,NULL,{}) failed: %s\n", + win32_channel->fd, emsg); + g_free (emsg); + } + + if (win32_channel->event) + if (!WSACloseEvent (win32_channel->event)) + if (win32_channel->debug) + { + gchar *emsg = g_win32_error_message (WSAGetLastError ()); + + g_print (" WSACloseEvent(%p) failed: %s\n", + win32_channel->event, emsg); + g_free (emsg); + } - g_free (win32_channel->buffer); g_free (win32_channel); } @@ -1280,7 +1331,7 @@ g_io_win32_fd_close (GIOChannel *channel, g_print ("g_io_win32_fd_close: thread=%#x: fd=%d\n", win32_channel->thread_id, win32_channel->fd); - LOCK (win32_channel->mutex); + EnterCriticalSection (&win32_channel->mutex); if (win32_channel->running) { if (win32_channel->debug) @@ -1303,7 +1354,7 @@ g_io_win32_fd_close (GIOChannel *channel, win32_channel->fd); win32_channel->fd = -1; } - UNLOCK (win32_channel->mutex); + LeaveCriticalSection (&win32_channel->mutex); /* FIXME error detection? */ @@ -1334,7 +1385,7 @@ g_io_win32_fd_create_watch (GIOChannel *channel, channel, win32_channel->fd, condition_to_string (condition), (HANDLE) watch->pollfd.fd); - LOCK (win32_channel->mutex); + EnterCriticalSection (&win32_channel->mutex); if (win32_channel->thread_id == 0) { if (condition & G_IO_IN) @@ -1344,7 +1395,7 @@ g_io_win32_fd_create_watch (GIOChannel *channel, } g_source_add_poll (source, &watch->pollfd); - UNLOCK (win32_channel->mutex); + LeaveCriticalSection (&win32_channel->mutex); return source; } @@ -1591,7 +1642,7 @@ g_io_channel_new_file (const gchar *filename, mode_num = MODE_A; break; default: - g_warning ("Invalid GIOFileMode %s.\n", mode); + g_warning ("Invalid GIOFileMode %s.", mode); return NULL; } @@ -1607,7 +1658,7 @@ g_io_channel_new_file (const gchar *filename, } /* Fall through */ default: - g_warning ("Invalid GIOFileMode %s.\n", mode); + g_warning ("Invalid GIOFileMode %s.", mode); return NULL; } -- 2.7.4