Re: File descriptor leaks from open() in e-d-s
Jeffrey Stedfast <[email protected]> Wed, 16 May 2007 09:40:39 -0400
| Newsgroups | gmane.comp.gnome.evolution.patches |
|---|---|
| Organization | Novell, Inc. |
| Message-ID | <1179322839.12751.59.camel@serenity> |
yea, I would say looks good to commit :) Jeff On Wed, 2007-05-16 at 12:39 +0200, Jules Colding wrote: > Hi, > > On Mon, 2007-05-14 at 10:21 -0400, Jeffrey Stedfast wrote: > > On Mon, 2007-05-14 at 15:44 +0200, Jules Colding wrote: > > Changed to g_mkstemp() everywhere possible. > > I this patch better? > > -- > jules > > > Index: camel/providers/nntp/ChangeLog > =================================================================== > --- camel/providers/nntp/ChangeLog (revision 7760) > +++ camel/providers/nntp/ChangeLog (working copy) > @@ -1,3 +1,7 @@ > +2007-05-14 Jules Colding <[email protected]> > + > + * camel-nntp-newsrc.c (camel_nntp_newsrc_read_for_server): Fix file descriptor leak > + > 2007-04-05 Ross Burton <[email protected]> > > * camel-nntp-folder.c: (camel_nntp_folder_new): > Index: camel/providers/nntp/camel-nntp-newsrc.c > =================================================================== > --- camel/providers/nntp/camel-nntp-newsrc.c (revision 7760) > +++ camel/providers/nntp/camel-nntp-newsrc.c (working copy) > @@ -622,6 +622,7 @@ > > if (fstat (fd, &sb) == -1) { > g_warning ("failed fstat on ~/.newsrc-%s: %s\n", server, strerror(errno)); > + close (fd); > return newsrc; > } > newsrc_len = sb.st_size; > Index: camel/providers/local/camel-spool-summary.c > =================================================================== > --- camel/providers/local/camel-spool-summary.c (revision 7760) > +++ camel/providers/local/camel-spool-summary.c (working copy) > @@ -128,7 +128,7 @@ > spool_summary_sync_full(CamelMboxSummary *cls, gboolean expunge, CamelFolderChangeInfo *changeinfo, CamelException *ex) > { > int fd = -1, fdout = -1; > - char *tmpname = NULL; > + char tmpname[64] = { '\0' }; > char *buffer, *p; > off_t spoollen, outlen; > int size, sizeout; > @@ -149,16 +149,9 @@ > return -1; > } > > -#ifdef HAVE_MKSTEMP > - tmpname = alloca (64); > sprintf (tmpname, "/tmp/spool.camel.XXXXXX"); > - fdout = mkstemp (tmpname); > -#else > -#warning "Your system has no mkstemp(3), spool updating may be insecure" > - tmpname = alloca (L_tmpnam); > - tmpnam (tmpname); > - fdout = open (tmpname, O_RDWR|O_CREAT|O_EXCL, 0600); > -#endif > + fdout = g_mkstemp (tmpname); > + > d(printf("Writing tmp file to %s\n", tmpname)); > if (fdout == -1) { > camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM, > @@ -245,11 +238,9 @@ > _("Could not sync spool folder %s: %s\n" > "Folder may be corrupt, copy saved in `%s'"), > ((CamelLocalSummary *)cls)->folder_path, > - g_strerror (errno), tmpnam); > + g_strerror (errno), tmpname); > /* so we dont delete it */ > - close(fdout); > - tmpname = NULL; > - fdout = -1; > + tmpname[0] = '\0'; > g_free(buffer); > goto error; > } > @@ -264,10 +255,8 @@ > _("Could not sync spool folder %s: %s\n" > "Folder may be corrupt, copy saved in `%s'"), > ((CamelLocalSummary *)cls)->folder_path, > - g_strerror (errno), tmpnam); > - close(fdout); > - tmpname = NULL; > - fdout = -1; > + g_strerror (errno), tmpname); > + tmpname[0] = '\0'; > goto error; > } > > @@ -277,17 +266,17 @@ > _("Could not sync spool folder %s: %s\n" > "Folder may be corrupt, copy saved in `%s'"), > ((CamelLocalSummary *)cls)->folder_path, > - g_strerror (errno), tmpnam); > - close(fdout); > - tmpname = NULL; > - fdout = -1; > + g_strerror (errno), tmpname); > + tmpname[0] = '\0'; > fd = -1; > goto error; > } > > close(fdout); > - unlink(tmpname); > > + if (tmpname[0] != '\0') > + unlink(tmpname); > + > camel_operation_end(NULL); > > return 0; > @@ -298,7 +287,7 @@ > if (fdout != -1) > close(fdout); > > - if (tmpname) > + if (tmpname[0] != '\0') > unlink(tmpname); > > camel_operation_end(NULL); > Index: camel/providers/local/camel-mh-summary.c > =================================================================== > --- camel/providers/local/camel-mh-summary.c (revision 7760) > +++ camel/providers/local/camel-mh-summary.c (working copy) > @@ -147,7 +147,8 @@ > } else { > /* else scan for one - and create it too, to make sure */ > do { > - close(fd); > + if (fd != -1) > + close(fd); > uid = camel_folder_summary_next_uid(s); > name = g_strdup_printf("%s/%u", cls->folder_path, uid); > /* O_EXCL isn't guaranteed, sigh. Oh well, bad luck, mh has problems anyway */ > @@ -155,7 +156,8 @@ > g_free(name); > } while (fd == -1 && errno == EEXIST); > > - close(fd); > + if (fd != -1) > + close(fd); > > uidstr = g_strdup_printf("%u", uid); > } > Index: camel/providers/local/ChangeLog > =================================================================== > --- camel/providers/local/ChangeLog (revision 7760) > +++ camel/providers/local/ChangeLog (working copy) > @@ -1,3 +1,16 @@ > +2007-05-16 Jules Colding <[email protected]> > + > + * camel-spool-summary.c (spool_summary_sync_full): Use g_mkstemp() > + > +2007-05-14 Jules Colding <[email protected]> > + > + * camel-mh-summary.c (mh_summary_next_uid_string): Do not close() if (fd == -1). > + > + * camel-spool-summary.c (spool_summary_sync_full): alloca() changed into malloc(). > + alloca() has undefined behaviour for stack overflow so it shouldn't really be used > + unless there is a *very* good reason. Cleanups related to this fix as well. Do not > + close() if (fd == -1). > + > 2007-04-05 Ross Burton <[email protected]> > > * camel-local-store.c: (get_folder): > Index: camel/ChangeLog > =================================================================== > --- camel/ChangeLog (revision 7760) > +++ camel/ChangeLog (working copy) > @@ -1,3 +1,27 @@ > +2007-05-16 Jules Colding <[email protected]> > + > + * camel-lock.c (camel_lock_dot): Use g_mkstemp() > + > + * camel-folder-summary.c (camel_folder_summary_save): Fix file descriptor leak. > + fclose() a fdopen() stream instead of close()'ing the underlying file descriptor. > + > + * camel-uid-cache.c (camel_uid_cache_save): Cleanups, such as > + setting cache->fd = -1 when fd has been closed. > + > + * camel-block-file.c (key_file_use): Do not close() if (fd == -1) > + > + * camel-index-control.c (do_perf): Fix file descriptor leak > + > + * camel-charset-map.c (main): Check for return value > + > + * camel-text-index.c (dump_raw): Fix file descriptor leak > + > + * camel-lock.c (main): Fix file descriptor leak. > + > + * camel-lock-client.c (camel_lock_helper_init): Fix file descriptor leak > + > + * camel-process.c (camel_process_fork): Fix file descriptor leak > + > 2007-05-15 Srinivasa Ragavan <[email protected]> > > > @@ -6,12 +30,6 @@ > * camel-disco-store.h: > > 2007-05-12 Jules Colding <[email protected]> > - > - * camel-lock-client.c (camel_lock_helper_init): Fix file descriptor leak > - > - * camel-process.c (camel_process_fork): Fix file descriptor leak > - > -2007-05-12 Jules Colding <[email protected]> > > * camel-store.h (CamelFolderInfo): Change type of unread and total > members to gint32 instead of guint32. The previous unsigned members > Index: camel/camel-lock.c > =================================================================== > --- camel/camel-lock.c (revision 7760) > +++ camel/camel-lock.c (working copy) > @@ -89,17 +89,6 @@ > sprintf(lock, "%s.lock", path); > locktmp = alloca(strlen(path) + strlen("XXXXXX") + 1); > > -#ifndef HAVE_MKSTEMP > - sprintf(locktmp, "%sXXXXXX", path); > - if (mktemp(locktmp) == NULL) { > - /* well, this is really only a programatic error */ > - camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM, > - _("Could not create lock file for %s: %s"), > - path, g_strerror (errno)); > - return -1; > - } > -#endif > - > while (retry < CAMEL_LOCK_DOT_RETRY) { > > d(printf("trying to lock '%s', attempt %d\n", lock, retry)); > @@ -107,12 +96,8 @@ > if (retry > 0) > sleep(CAMEL_LOCK_DOT_DELAY); > > -#ifdef HAVE_MKSTEMP > sprintf(locktmp, "%sXXXXXX", path); > - fdtmp = mkstemp(locktmp); > -#else > - fdtmp = open(locktmp, O_RDWR|O_CREAT|O_EXCL, 0600); > -#endif > + fdtmp = g_mkstemp(locktmp); > if (fdtmp == -1) { > camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM, > _("Could not create lock file for %s: %s"), > @@ -372,7 +357,16 @@ > #endif > > fd1 = open("mylock", O_RDWR); > + if (fd1 == -1) { > + printf("Could not open lock file (mylock): %s", g_strerror (errno)); > + return 1; > + } > fd2 = open("mylock", O_RDWR); > + if (fd2 == -1) { > + printf("Could not open lock file (mylock): %s", g_strerror (errno)); > + close (fd1); > + return 1; > + } > > if (camel_lock_fcntl(fd1, CAMEL_LOCK_WRITE, ex) == 0) { > printf("got fcntl write lock once\n"); > Index: camel/tests/message/test2.c > =================================================================== > --- camel/tests/message/test2.c (revision 7760) > +++ camel/tests/message/test2.c (working copy) > @@ -52,6 +52,9 @@ > char *nout, *noutp; > iconv_t ic = iconv_open(from, to); > > + if (ic == (iconv_t)-1) > + goto fail; > + > inp = out; > inlen = strlen(out); > outlen = inlen*5 + 16; > @@ -68,6 +71,7 @@ > printf("Camel thinks the best encoding of '%s' is %s, although we converted from %s\n", > in, camel_charset_best(out, strlen(out)), from); > } > +fail: > #endif > > return out; > Index: camel/tests/ChangeLog > =================================================================== > --- camel/tests/ChangeLog (revision 7760) > +++ camel/tests/ChangeLog (working copy) > @@ -1,3 +1,9 @@ > +2007-05-14 Jules Colding <[email protected]> > + > + * mime-filter/test-charset.c (main): Check return value > + > + * message/test2.c (convert): Check return value > + > 2006-06-22 Harish Krishnaswamy <[email protected]> > > * Makefile.am: Remove CVS/*, .cvsignore files from > Index: camel/tests/mime-filter/test-charset.c > =================================================================== > --- camel/tests/mime-filter/test-charset.c (revision 7760) > +++ camel/tests/mime-filter/test-charset.c (working copy) > @@ -36,6 +36,8 @@ > camel_test_init(argc, argv); > > dir = opendir (SOURCEDIR); > + if (!dir) > + return 1; > > while ((dent = readdir (dir))) { > char *infile, *outfile, *charset, *work; > Index: camel/camel-text-index.c > =================================================================== > --- camel/camel-text-index.c (revision 7760) > +++ camel/camel-text-index.c (working copy) > @@ -1172,6 +1172,7 @@ > } while (len); > printf("\n"); > } > + close (fd); > } > #endif > > @@ -1937,6 +1938,7 @@ > > index++; > } > + fclose (fp); > > printf("Freeing partition index\n"); > camel_partition_table_free(cpi); > Index: camel/camel-charset-map.c > =================================================================== > --- camel/camel-charset-map.c (revision 7760) > +++ camel/camel-charset-map.c (working copy) > @@ -106,6 +106,8 @@ > > for (j = 0; tables[j].name; j++) { > cd = iconv_open (UCS, tables[j].name); > + if (cd == (iconv_t)-1) > + exit (1); > inptr = in; > outptr = (char *)(out); > inlen = sizeof (in); > Index: camel/camel-index-control.c > =================================================================== > --- camel/camel-index-control.c (revision 7760) > +++ camel/camel-index-control.c (working copy) > @@ -172,6 +172,7 @@ > idx = (CamelIndex *)camel_text_index_new("/tmp/index", O_TRUNC|O_CREAT|O_RDWR); > if (idx == NULL) { > perror("open index"); > + closedir(dir); > return 1; > } > > Index: camel/camel-block-file.c > =================================================================== > --- camel/camel-block-file.c (revision 7760) > +++ camel/camel-block-file.c (working copy) > @@ -916,7 +916,8 @@ > if ((fd = g_open(bs->path, bs->flags|O_BINARY, 0600)) == -1 > || (bs->fp = fdopen(fd, flag)) == NULL) { > err = errno; > - close(fd); > + if (fd != -1) > + close(fd); > CAMEL_KEY_FILE_UNLOCK(bs, lock); > errno = err; > return -1; > Index: camel/camel-uid-cache.c > =================================================================== > --- camel/camel-uid-cache.c (revision 7760) > +++ camel/camel-uid-cache.c (working copy) > @@ -169,15 +169,13 @@ > cache->expired = 0; > g_hash_table_foreach (cache->uids, maybe_write_uid, cache); > > - if (cache->fd == -1) > - goto exception; > - > if (fsync (fd) == -1) > goto exception; > > close (fd); > fd = -1; > - > + cache->fd = -1; > + > if (g_rename (filename, cache->filename) == -1) > goto exception; > > @@ -211,14 +209,16 @@ > if (ftruncate (fd, (off_t) cache->size) != -1) { > cache->size = 0; > cache->expired = 0; > - goto overwrite; > + goto overwrite; /* FIXME: no such label */ > } > - } > - > + } > + } > +#endif > + if (fd != -1) { > close (fd); > + cache->fd = -1; > } > -#endif > - > + > g_unlink (filename); > g_free (filename); > > Index: camel/camel-folder-summary.c > =================================================================== > --- camel/camel-folder-summary.c (revision 7760) > +++ camel/camel-folder-summary.c (working copy) > @@ -687,14 +687,16 @@ > path_meta = alloca(strlen(s->meta_summary->path)+4); > sprintf(path_meta, "%s~", s->meta_summary->path); > fd_meta = g_open(path_meta, O_RDWR|O_CREAT|O_TRUNC|O_BINARY, 0600); > - if (fd_meta == -1) > + if (fd_meta == -1) { > + fclose(out); > return -1; > + } > out_meta = fdopen(fd_meta, "wb"); > if (out_meta == NULL) { > i = errno; > g_unlink(path); > g_unlink(path_meta); > - close(fd); > + fclose(out); > close(fd_meta); > errno = i; > return -1; > Index: ChangeLog > =================================================================== > --- ChangeLog (revision 7760) > +++ ChangeLog (working copy) > @@ -1,3 +1,9 @@ > +2007-05-14 Jules Colding <[email protected]> > + > + * libedataserver/e-db3-utils.c (cp_file): Cleanup. Fix file descriptor leak. > + > + * libedataserver/md5-utils.c (md5_get_digest_from_file): Fix file descriptor leak > + > 2007-05-14 Srinivasa Ragavan > > * NEWS, configure.in: Evolution Data Server 1.11.2 release. > Index: libedataserver/md5-utils.c > =================================================================== > --- libedataserver/md5-utils.c (revision 7760) > +++ libedataserver/md5-utils.c (working copy) > @@ -340,7 +340,7 @@ > md5_init (&ctx); > fp = g_fopen(filename, "rb"); > if (!fp) { > - return; > + return; > } > > while ((nb_bytes_read = fread (tmp_buf, 1, sizeof (tmp_buf), fp)) > 0) > @@ -350,7 +350,7 @@ > fclose(fp); > return; > } > - > + fclose(fp); > > md5_final (&ctx, digest); > } > Index: libedataserver/e-xml-utils.c > =================================================================== > --- libedataserver/e-xml-utils.c (revision 7760) > +++ libedataserver/e-xml-utils.c (working copy) > @@ -107,7 +107,7 @@ > errno = errnosave; > return -1; > } > - > + > while ((ret = close (fd)) == -1 && errno == EINTR) > ; > > Index: libedataserver/e-db3-utils.c > =================================================================== > --- libedataserver/e-db3-utils.c (revision 7760) > +++ libedataserver/e-db3-utils.c (working copy) > @@ -83,11 +83,10 @@ > place += count; > } > } > - if (close (i)) > - return -1; > - if (close (o)) > - return -1; > - return 0; > + i = close (i); > + if (close (o) == -1) > + i = -1; > + return (i == -1) ? -1 : 0; > } > > static int > @@ -98,10 +97,7 @@ > if (o == -1) > return -1; > > - if (close (o) == -1) > - return -1; > - > - return 0; > + return close (o); > } > > static int > Index: addressbook/tests/vcard/dump-vcard.c > =================================================================== > --- addressbook/tests/vcard/dump-vcard.c (revision 7760) > +++ addressbook/tests/vcard/dump-vcard.c (working copy) > @@ -23,6 +23,7 @@ > if (fgets (buf, sizeof(buf), fp)) > str = g_string_append (str, buf); > } > + fclose (fp); > > vcard = e_vcard_new_from_string (str->str); > > Index: addressbook/ChangeLog > =================================================================== > --- addressbook/ChangeLog (revision 7760) > +++ addressbook/ChangeLog (working copy) > @@ -1,3 +1,10 @@ > +2007-05-16 Jules Colding <[email protected]> > + > + * backends/vcf/e-book-backend-vcf.c (load_file): Fix file descriptor leak. > + (save_file): Cleanup. Fix file descriptor leak. > + > + * tests/vcard/dump-vcard.c (main): Fix file descriptor leak > + > 2007-05-15 Ross Burton <[email protected]> > > * tests/ebook/test-ebook-async.c: > Index: addressbook/backends/vcf/e-book-backend-vcf.c > =================================================================== > --- addressbook/backends/vcf/e-book-backend-vcf.c (revision 7760) > +++ addressbook/backends/vcf/e-book-backend-vcf.c (working copy) > @@ -110,6 +110,7 @@ > > fp = fdopen (fd, "rb"); > if (!fp) { > + close (fd); /* callers depend on fd being closed by this function */ > g_warning ("failed to open `%s' for reading", vcf->priv->filename); > return; > } > @@ -140,6 +141,7 @@ > static gboolean > save_file (EBookBackendVCF *vcf) > { > + gboolean retv = FALSE; > GList *l; > char *new_path; > int fd, rv; > @@ -151,6 +153,10 @@ > new_path = g_strdup_printf ("%s.new", vcf->priv->filename); > > fd = g_open (new_path, O_CREAT | O_TRUNC | O_WRONLY | O_BINARY, 0666); > + if (fd == -1) { > + g_warning ("write failed. could not open output file\n"); > + goto out; > + } > > for (l = vcf->priv->contact_list; l; l = l->next) { > char *vcard_str = l->data; > @@ -161,34 +167,34 @@ > if (rv < len) { > /* XXX */ > g_warning ("write failed. we need to handle short writes\n"); > - close (fd); > g_unlink (new_path); > - return FALSE; > + goto out; > } > > rv = write (fd, "\r\n\r\n", 4); > if (rv < 4) { > /* XXX */ > g_warning ("write failed. we need to handle short writes\n"); > - close (fd); > g_unlink (new_path); > - return FALSE; > + goto out; > } > } > > if (0 > g_rename (new_path, vcf->priv->filename)) { > g_warning ("Failed to rename %s: %s\n", vcf->priv->filename, strerror(errno)); > g_unlink (new_path); > - return FALSE; > + goto out; > } > + retv = TRUE; > > +out: > + if (fd != -1) > + close (fd); > g_free (new_path); > - > - vcf->priv->dirty = FALSE; > - > + vcf->priv->dirty = !retv; > g_mutex_unlock (vcf->priv->mutex); > > - return TRUE; > + return retv; > } > > static gboolean > Index: calendar/ChangeLog > =================================================================== > --- calendar/ChangeLog (revision 7760) > +++ calendar/ChangeLog (working copy) > @@ -1,3 +1,14 @@ > +2007-05-16 Jules Colding <[email protected]> > + > + * backends/groupwise/e-cal-backend-groupwise.c (fetch_attachments): > + Do not close() if (fd == -1) > + > + * backends/groupwise/e-cal-backend-groupwise-utils.c (set_attachments_to_cal_component): > + Do not close() if (fd == -1) > + > + * backends/file/e-cal-backend-file.c (fetch_attachments): > + Do not close() if (fd == -1) > + > 2007-05-15 Ross Burton <[email protected]> > > * tests/ecal/test-ecal.c: > Index: calendar/backends/file/e-cal-backend-file.c > =================================================================== > --- calendar/backends/file/e-cal-backend-file.c (revision 7760) > +++ calendar/backends/file/e-cal-backend-file.c (working copy) > @@ -2352,7 +2352,8 @@ > } > > g_mapped_file_free (mapped_file); > - close (fd); > + if (fd != -1) > + close (fd); > dest_url = g_filename_to_uri (dest_file, NULL, NULL); > g_free (dest_file); > new_attach_list = g_slist_append (new_attach_list, dest_url); > Index: calendar/backends/groupwise/e-cal-backend-groupwise-utils.c > =================================================================== > --- calendar/backends/groupwise/e-cal-backend-groupwise-utils.c (revision 7760) > +++ calendar/backends/groupwise/e-cal-backend-groupwise-utils.c (working copy) > @@ -833,7 +833,8 @@ > g_warning ("DEBUG: attachment write failed.\n"); > } > g_free (attach_data); > - close (fd); > + if (fd != -1) > + close (fd); > } > g_free (filename); > > Index: calendar/backends/groupwise/e-cal-backend-groupwise.c > =================================================================== > --- calendar/backends/groupwise/e-cal-backend-groupwise.c (revision 7760) > +++ calendar/backends/groupwise/e-cal-backend-groupwise.c (working copy) > @@ -2279,7 +2279,8 @@ > } > > g_mapped_file_free (mapped_file); > - close (fd); > + if (fd != -1) > + close (fd); > dest_url = g_filename_to_uri (dest_file, NULL, NULL); > g_free (dest_file); > new_attach_list = g_slist_append (new_attach_list, dest_url); > >