{"thread":{"id":"61847","subject":"[PATCH] csum-file: introduce discard_hashfile()","startedAt":"2024-07-25T23:07:31Z","lastAt":"2024-07-26T15:36:22Z","messageCount":5,"participants":["Junio C Hamano","Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"499380","messageId":"xmqqle1p1367.fsf@gitster.g","threadId":"61847","inReplyTo":null,"subject":"[PATCH] csum-file: introduce discard_hashfile()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-25T23:07:28Z","receivedAt":"2024-07-25T23:07:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The hashfile API is used to write out a \"hashfile\", which has a\nfinal checksum (typically SHA-1) at the end.  An in-core hashfile\nstructure has up to two file descriptors and a few buffers that can\nonly be freed by calling a helper function that is private to the\ncsum-file implementation.\n\nThe usual flow of a user of the API is to first open a file\ndescriptor for writing, obtain a hashfile associated with that write\nfile descriptor by calling either hashfd() or hashfd_check(), call\nhashwrite() number of times to write data to the file, and then call\nfinalize_hashfile(), which appends th checksum to the end of the\nfile, closes file descriptors and releases associated buffers.\n\nBut what if a caller finds some error after calling hashfd() to\nstart the process and/or hashwrite() to send some data to the file,\nand wants to abort the operation?  The underlying file descriptor is\noften managed by the tempfile API, so aborting will clean the file\nout of the filesystem, but the resources associated with the in-core\nhashfile structure is lost.\n\nIntroduce discard_hashfile() API function to allow them to release\nthe resources held by a hashfile structure the callers want to\ndispose of, and use that in read-cache.c:do_write_index(), which is\na central place that writes the index file.\n\nMark t2107 as leak-free, as this leak in \"update-index --cacheinfo\"\ntest that deliberately makes it fail is now plugged.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n csum-file.c                   |  9 ++++\n csum-file.h                   |  1 +\n read-cache.c                  | 99 +++++++++++++++++++++++++++--------\n t/t2107-update-index-basic.sh |  1 +\n 4 files changed, 89 insertions(+), 21 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 8abbf01325..2131ee6b12 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -102,6 +102,15 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n \treturn fd;\n }\n \n+void discard_hashfile(struct hashfile *f)\n+{\n+\tif (0 <= f->check_fd)\n+\t\tclose(f->check_fd);\n+\tif (0 <= f->fd)\n+\t\tclose(f->fd);\n+\tfree_hashfile(f);\n+}\n+\n void hashwrite(struct hashfile *f, const void *buf, unsigned int count)\n {\n \twhile (count) {\ndiff --git a/csum-file.h b/csum-file.h\nindex 566e05cbd2..36c7c5585f 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -47,6 +47,7 @@ struct hashfile *hashfd(int fd, const char *name);\n struct hashfile *hashfd_check(const char *name);\n struct hashfile *hashfd_throughput(int fd, const char *name, struct progress *tp);\n int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_component, unsigned int);\n+void discard_hashfile(struct hashfile *);\n void hashwrite(struct hashfile *, const void *, unsigned int);\n void hashflush(struct hashfile *f);\n void crc32_begin(struct hashfile *);\ndiff --git a/read-cache.c b/read-cache.c\nindex 48bf24f87c..d96a2cb8cf 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2963,7 +2963,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \tif (err) {\n \t\tfree(ieot);\n-\t\treturn err;\n+\t\tgoto discard_hashfile_and_return;\n \t}\n \n \toffset = hashfile_total(f);\n@@ -2992,8 +2992,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n \t\tfree(ieot);\n-\t\tif (err)\n-\t\t\treturn -1;\n+\t\t/*\n+\t\t * NEEDSWORK: write_index_ext_header() never returns a failure,\n+\t\t * and this part may want to be simplified.\n+\t\t */\n+\t\tif (err) {\n+\t\t\terr = -1;\n+\t\t\tgoto discard_hashfile_and_return;\n+\t\t}\n \t}\n \n \tif (write_extensions & WRITE_SPLIT_INDEX_EXTENSION &&\n@@ -3008,8 +3014,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t\t\t       sb.len) < 0;\n \t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n-\t\tif (err)\n-\t\t\treturn -1;\n+\t\t/*\n+\t\t * NEEDSWORK: write_link_extension() never returns a failure,\n+\t\t * and this part may want to be simplified.\n+\t\t */\n+\t\tif (err) {\n+\t\t\terr = -1;\n+\t\t\tgoto discard_hashfile_and_return;\n+\t\t}\n \t}\n \tif (write_extensions & WRITE_CACHE_TREE_EXTENSION &&\n \t    !drop_cache_tree && istate->cache_tree) {\n@@ -3019,8 +3031,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_TREE, sb.len) < 0;\n \t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n-\t\tif (err)\n-\t\t\treturn -1;\n+\t\t/*\n+\t\t * NEEDSWORK: write_index_ext_header() never returns a failure,\n+\t\t * and this part may want to be simplified.\n+\t\t */\n+\t\tif (err) {\n+\t\t\terr = -1;\n+\t\t\tgoto discard_hashfile_and_return;\n+\t\t}\n \t}\n \tif (write_extensions & WRITE_RESOLVE_UNDO_EXTENSION &&\n \t    istate->resolve_undo) {\n@@ -3031,8 +3049,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t\t\t     sb.len) < 0;\n \t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n-\t\tif (err)\n-\t\t\treturn -1;\n+\t\t/*\n+\t\t * NEEDSWORK: write_index_ext_header() never returns a failure,\n+\t\t * and this part may want to be simplified.\n+\t\t */\n+\t\tif (err) {\n+\t\t\terr = -1;\n+\t\t\tgoto discard_hashfile_and_return;\n+\t\t}\n \t}\n \tif (write_extensions & WRITE_UNTRACKED_CACHE_EXTENSION &&\n \t    istate->untracked) {\n@@ -3043,8 +3067,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t\t\t     sb.len) < 0;\n \t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n-\t\tif (err)\n-\t\t\treturn -1;\n+\t\t/*\n+\t\t * NEEDSWORK: write_index_ext_header() never returns a failure,\n+\t\t * and this part may want to be simplified.\n+\t\t */\n+\t\tif (err) {\n+\t\t\terr = -1;\n+\t\t\tgoto discard_hashfile_and_return;\n+\t\t}\n \t}\n \tif (write_extensions & WRITE_FSMONITOR_EXTENSION &&\n \t    istate->fsmonitor_last_update) {\n@@ -3054,12 +3084,25 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_FSMONITOR, sb.len) < 0;\n \t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n-\t\tif (err)\n-\t\t\treturn -1;\n+\t\t/*\n+\t\t * NEEDSWORK: write_index_ext_header() never returns a failure,\n+\t\t * and this part may want to be simplified.\n+\t\t */\n+\t\tif (err) {\n+\t\t\terr = -1;\n+\t\t\tgoto discard_hashfile_and_return;\n+\t\t}\n \t}\n \tif (istate->sparse_index) {\n-\t\tif (write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0) < 0)\n-\t\t\treturn -1;\n+\t\terr = write_index_ext_header(f, eoie_c, CACHE_EXT_SPARSE_DIRECTORIES, 0);\n+\t\t/*\n+\t\t * NEEDSWORK: write_index_ext_header() never returns a failure,\n+\t\t * and this part may want to be simplified.\n+\t\t */\n+\t\tif (err) {\n+\t\t\terr = -1;\n+\t\t\tgoto discard_hashfile_and_return;\n+\t\t}\n \t}\n \n \t/*\n@@ -3075,8 +3118,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\terr = write_index_ext_header(f, NULL, CACHE_EXT_ENDOFINDEXENTRIES, sb.len) < 0;\n \t\thashwrite(f, sb.buf, sb.len);\n \t\tstrbuf_release(&sb);\n-\t\tif (err)\n-\t\t\treturn -1;\n+\t\t/*\n+\t\t * NEEDSWORK: write_index_ext_header() never returns a failure,\n+\t\t * and this part may want to be simplified.\n+\t\t */\n+\t\tif (err) {\n+\t\t\terr = -1;\n+\t\t\tgoto discard_hashfile_and_return;\n+\t\t}\n \t}\n \n \tcsum_fsync_flag = 0;\n@@ -3085,13 +3134,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \n \tfinalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_INDEX,\n \t\t\t  CSUM_HASH_IN_STREAM | csum_fsync_flag);\n+\tf = NULL;\n \n \tif (close_tempfile_gently(tempfile)) {\n-\t\terror(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n-\t\treturn -1;\n+\t\terr = error(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n+\t\tgoto discard_hashfile_and_return;\n+\t}\n+\tif (stat(get_tempfile_path(tempfile), &st)) {\n+\t\terr = error_errno(_(\"could not stat '%s'\"), get_tempfile_path(tempfile));\n+\t\tgoto discard_hashfile_and_return;\n \t}\n-\tif (stat(get_tempfile_path(tempfile), &st))\n-\t\treturn -1;\n \tistate->timestamp.sec = (unsigned int)st.st_mtime;\n \tistate->timestamp.nsec = ST_MTIME_NSEC(st);\n \ttrace_performance_since(start, \"write index, changed mask = %x\", istate->cache_changed);\n@@ -3106,6 +3158,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t   istate->cache_nr);\n \n \treturn 0;\n+\n+discard_hashfile_and_return:\n+\tif (f)\n+\t\tdiscard_hashfile(f);\n+\treturn err;\n }\n \n void set_alternate_index_output(const char *name)\ndiff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\nindex cc72ead79f..f0eab13f96 100755\n--- a/t/t2107-update-index-basic.sh\n+++ b/t/t2107-update-index-basic.sh\n@@ -5,6 +5,7 @@ test_description='basic update-index tests\n Tests for command-line parsing and basic operation.\n '\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'update-index --nonsense fails' '\n-- \n2.46.0-rc2-67-g99767055c1\n\n"},{"id":"499387","messageId":"20240726044216.GA642208@coredump.intra.peff.net","threadId":"61847","inReplyTo":"xmqqle1p1367.fsf@gitster.g","subject":"Re: [PATCH] csum-file: introduce discard_hashfile()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-26T04:42:16Z","receivedAt":"2024-07-26T04:42:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 25, 2024 at 04:07:28PM -0700, Junio C Hamano wrote:\n\n> Introduce discard_hashfile() API function to allow them to release\n> the resources held by a hashfile structure the callers want to\n> dispose of, and use that in read-cache.c:do_write_index(), which is\n> a central place that writes the index file.\n\nNicely explained, and the patch looks good to me.\n\nA few small comments (that probably do not need any changes):\n\n> +void discard_hashfile(struct hashfile *f)\n> +{\n> +\tif (0 <= f->check_fd)\n> +\t\tclose(f->check_fd);\n> +\tif (0 <= f->fd)\n> +\t\tclose(f->fd);\n> +\tfree_hashfile(f);\n> +}\n\nI wondered if we could call this function to replace other spots that\nclose the descriptors. But I don't think so. There is only one such\nspot, in finalize_hashfile(), and it does extra work to make sure that\nthe close succeeds. So it wouldn't make sense to convert, and nobody\nelse (until now) bothered to clean up a discarded hashfile.\n\n> diff --git a/read-cache.c b/read-cache.c\n> index 48bf24f87c..d96a2cb8cf 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2963,7 +2963,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>  \n>  \tif (err) {\n>  \t\tfree(ieot);\n> -\t\treturn err;\n> +\t\tgoto discard_hashfile_and_return;\n>  \t}\n>  \n>  \toffset = hashfile_total(f);\n> @@ -2992,8 +2992,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>  \t\thashwrite(f, sb.buf, sb.len);\n>  \t\tstrbuf_release(&sb);\n>  \t\tfree(ieot);\n> -\t\tif (err)\n> -\t\t\treturn -1;\n> +\t\t/*\n> +\t\t * NEEDSWORK: write_index_ext_header() never returns a failure,\n> +\t\t * and this part may want to be simplified.\n> +\t\t */\n> +\t\tif (err) {\n> +\t\t\terr = -1;\n> +\t\t\tgoto discard_hashfile_and_return;\n> +\t\t}\n>  \t}\n\nThere's other repeated cleanup happening here, like free(ieot) and\nstrbuf_release(), which made wonder if we could bump it down to the\ncleanup label at the end of the function to simplify things. But\nprobably not, as we are often doing that cleanup even in the non-error\ncase. And of course the \"sb\" strbuf is local to a lot of blocks.\n\nSo even if we did want to do it, I think it would come as a separate\npatch. But mostly I wondered whether the label should be a more generic\n\"cleanup\" than \"discard_hashfile\". We could probably worry about that\nlater, though, if that separate patch ever materializes.\n\n> @@ -3106,6 +3158,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>  \t\t\t   istate->cache_nr);\n>  \n>  \treturn 0;\n> +\n> +discard_hashfile_and_return:\n> +\tif (f)\n> +\t\tdiscard_hashfile(f);\n> +\treturn err;\n>  }\n\nOK, so here's our cleanup label. We check that \"f\" is valid. I notice we\ndon't initialize it to NULL, but assigning to it from hashfd() is the\nvery first thing we do, so it will never be uninitialized. Good.\n\nThat made me wonder when it would ever be NULL, and the answer is that\nit becomes so after we finalize it:\n\n> @@ -3085,13 +3134,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>  \n>  \tfinalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_INDEX,\n>  \t\t\t  CSUM_HASH_IN_STREAM | csum_fsync_flag);\n> +\tf = NULL;\n\nwhich makes sense. Arguably finalize_hashfile() could take a\npointer-to-pointer and set it to NULL itself for safety/simplicity, but\nit's probably OK to let the caller deal with it (we do that trick in the\ntempfile API, but not elsewhere).\n\nOne thing I did notice. The full hunk above is:\n\n> @@ -3085,13 +3134,16 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>  \n>  \tfinalize_hashfile(f, istate->oid.hash, FSYNC_COMPONENT_INDEX,\n>  \t\t\t  CSUM_HASH_IN_STREAM | csum_fsync_flag);\n> +\tf = NULL;\n>  \n>  \tif (close_tempfile_gently(tempfile)) {\n> -\t\terror(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n> -\t\treturn -1;\n> +\t\terr = error(_(\"could not close '%s'\"), get_tempfile_path(tempfile));\n> +\t\tgoto discard_hashfile_and_return;\n> +\t}\n> +\tif (stat(get_tempfile_path(tempfile), &st)) {\n> +\t\terr = error_errno(_(\"could not stat '%s'\"), get_tempfile_path(tempfile));\n> +\t\tgoto discard_hashfile_and_return;\n>  \t}\n> -\tif (stat(get_tempfile_path(tempfile), &st))\n> -\t\treturn -1;\n\nWe know that \"f\" is always NULL at this point, so the new code at the\ndiscard_hashfile_and_return label will never actually free anything.\nWe could continue to just \"return -1\" here and be fine. I am OK with it\nto keep the general \"jump to the cleanup label and return\" pattern\nconsistent within the function, but it would make more sense if the\nlabel had a less specific name. ;)\n\n-Peff\n"},{"id":"499412","messageId":"ZqORDmTj4GT0Eeqq@tanuki","threadId":"61847","inReplyTo":"20240726044216.GA642208@coredump.intra.peff.net","subject":"Re: [PATCH] csum-file: introduce discard_hashfile()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-26T12:05:45Z","receivedAt":"2024-07-26T12:05:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jul 26, 2024 at 12:42:16AM -0400, Jeff King wrote:\n> On Thu, Jul 25, 2024 at 04:07:28PM -0700, Junio C Hamano wrote:\n> \n> > Introduce discard_hashfile() API function to allow them to release\n> > the resources held by a hashfile structure the callers want to\n> > dispose of, and use that in read-cache.c:do_write_index(), which is\n> > a central place that writes the index file.\n> \n> Nicely explained, and the patch looks good to me.\n> \n> A few small comments (that probably do not need any changes):\n> \n> > +void discard_hashfile(struct hashfile *f)\n> > +{\n> > +\tif (0 <= f->check_fd)\n> > +\t\tclose(f->check_fd);\n> > +\tif (0 <= f->fd)\n> > +\t\tclose(f->fd);\n> > +\tfree_hashfile(f);\n> > +}\n\nAre we sure that this is always correct? A valid file descriptor may\nhave a zero value, and we wouldn't end up closing it here.\n\n> > @@ -2992,8 +2992,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n> >  \t\thashwrite(f, sb.buf, sb.len);\n> >  \t\tstrbuf_release(&sb);\n> >  \t\tfree(ieot);\n> > -\t\tif (err)\n> > -\t\t\treturn -1;\n> > +\t\t/*\n> > +\t\t * NEEDSWORK: write_index_ext_header() never returns a failure,\n> > +\t\t * and this part may want to be simplified.\n> > +\t\t */\n> > +\t\tif (err) {\n> > +\t\t\terr = -1;\n> > +\t\t\tgoto discard_hashfile_and_return;\n> > +\t\t}\n> >  \t}\n> \n> There's other repeated cleanup happening here, like free(ieot) and\n> strbuf_release(), which made wonder if we could bump it down to the\n> cleanup label at the end of the function to simplify things. But\n> probably not, as we are often doing that cleanup even in the non-error\n> case. And of course the \"sb\" strbuf is local to a lot of blocks.\n> \n> So even if we did want to do it, I think it would come as a separate\n> patch. But mostly I wondered whether the label should be a more generic\n> \"cleanup\" than \"discard_hashfile\". We could probably worry about that\n> later, though, if that separate patch ever materializes.\n\nIndeed, I wanted to say the same. I've got a patch series sitting around\nlocally where I do this. I guess I should send out my memory leak fixes\nsooner rather than later to avoid duplicated work :)\n\nPatrick\n"},{"id":"499444","messageId":"xmqqplr0z051.fsf@gitster.g","threadId":"61847","inReplyTo":"20240726044216.GA642208@coredump.intra.peff.net","subject":"Re: [PATCH] csum-file: introduce discard_hashfile()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-26T14:41:14Z","receivedAt":"2024-07-26T14:41:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There's other repeated cleanup happening here, like free(ieot) and\n> strbuf_release(), which made wonder if we could bump it down to the\n> cleanup label at the end of the function to simplify things. But\n> probably not, as we are often doing that cleanup even in the non-error\n> case. And of course the \"sb\" strbuf is local to a lot of blocks.\n\nThese localized and independent strbuf instances were indeed what\ndiscouraged me from moving other clean-up to a central place.\n\n> So even if we did want to do it, I think it would come as a separate\n> patch. But mostly I wondered whether the label should be a more generic\n> \"cleanup\" than \"discard_hashfile\". We could probably worry about that\n> later, though, if that separate patch ever materializes.\n\nYup, I wobbled between a more generic \"cleanup\" and \"hashfile is the\nonly thing that needs special clean-up right now\", and it does show,\nas you noticed, how the error code paths after calling finalize\nlooks like.\n\nI think I'll rename the label to \"cleaup\".\n\nThanks.\n"},{"id":"499450","messageId":"xmqqplr0xj0u.fsf@gitster.g","threadId":"61847","inReplyTo":"ZqORDmTj4GT0Eeqq@tanuki","subject":"Re: [PATCH] csum-file: introduce discard_hashfile()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-26T15:36:17Z","receivedAt":"2024-07-26T15:36:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Fri, Jul 26, 2024 at 12:42:16AM -0400, Jeff King wrote:\n>> On Thu, Jul 25, 2024 at 04:07:28PM -0700, Junio C Hamano wrote:\n>> \n>> > Introduce discard_hashfile() API function to allow them to release\n>> > the resources held by a hashfile structure the callers want to\n>> > dispose of, and use that in read-cache.c:do_write_index(), which is\n>> > a central place that writes the index file.\n>> \n>> Nicely explained, and the patch looks good to me.\n>> \n>> A few small comments (that probably do not need any changes):\n>> \n>> > +void discard_hashfile(struct hashfile *f)\n>> > +{\n>> > +\tif (0 <= f->check_fd)\n>> > +\t\tclose(f->check_fd);\n>> > +\tif (0 <= f->fd)\n>> > +\t\tclose(f->fd);\n>> > +\tfree_hashfile(f);\n>> > +}\n>\n> Are we sure that this is always correct? A valid file descriptor may\n> have a zero value, and we wouldn't end up closing it here.\n\nI thought that these two fd members use -1 for their \"zero value\"\nfor that exact reason.\n"}]}