fixes use of mkstemp and associated file-descriptor-leaking behavior #529

samhatchett · 2019-09-18T17:08:43Z

I was pretty surprised to find this behavior - but it showed up while running lots of independent simulations serially under the same process - the OS (Mac/*nix) said it reached the open-file limit. Investigation revealed that mkstemp returns a file descriptor ready for r/w - even though epanet later opens this file under a different handle, closes the second handle, and removes the file - without closing the (leaked) descriptor.

This workaround uses mkstemp to only create the file name from the template provided. The temp file created is closed and unlinked.

LRossman · 2019-09-18T19:46:17Z

Just out of curiosity, do the files stay opened after your app terminates, thereby preventing file access by any newly launched app or does the OS free all of them?

samhatchett · 2019-09-19T00:43:01Z

The OS will clean up the file descriptors in some way that is mysterious to me - maybe just because the process owns them, and when the process dies the descriptors go away. In any case on Mac, the ulimit is configurable but by default is pretty low.

LRossman · 2019-09-25T15:47:21Z

@samhatchett was there any specific reason for #including unistd.h in the changes made to project.c? This header file, which provides access to POSIX, doesn't seem to be needed for the new lines that were added to the getTmpName() function and is not included with MS VC.

samhatchett · 2019-09-25T16:07:03Z

@LRossman you are right - I think I started to use unlink which is posix, but replaced it with the more-standard remove call. I will revert that include line. Thanks!

samhatchett · 2019-09-25T16:08:54Z

#533

samhatchett · 2019-09-25T18:22:14Z

actually @LRossman - turns out that removing unistd.h gives a warning:

warning: implicit declaration of function 'close' is invalid in C99

LRossman · 2019-09-25T19:27:12Z

How about replacing the current code with this:

    strcpy(fname, "enXXXXXX");
    FILE *f = fdopen(mkstemp(fname), "r");
    fclose(f);
    remove(fname);

which should compile with just using stdio.h.

samhatchett · 2019-09-25T19:30:00Z

this would work, as would conditional inclusion of unistd.h - since the close-containing code is only compiled on *nix-like systems anyway. which seems simpler?

LRossman · 2019-09-25T20:02:43Z

Using fdopen seems simpler than adding an #ifdef statement for including unistd.h and thus introducing POSIX compatibility that is not required anywhere else in the code.

Also I wonder if the remove(fname) line can be deleted since each of the three temporary files whose names are generated by getTmpName(), are removed when a project is deleted, even if they are never opened by the toolkit.

samhatchett · 2019-09-26T00:24:12Z

may I draw your attention to the current state of the file and function in question...

given this precedent, I'd almost suggest putting <unistd.h> in an else clause just beyond <windows.h> - this might be least intrusive to the existing style 😉

EPANET/src/project.c

Line 1068 in d111fbf

char *getTmpName(char *fname)

⬇️

//*** For the Windows SDK _tempnam function ***//
#ifdef _WIN32
#include <windows.h>
#endif

char *getTmpName(char *fname)
//----------------------------------------------------------------
//  Input:   fname = file name string
//  Output:  returns pointer to file name
//  Purpose: creates a temporary file name with path prepended to it.
//----------------------------------------------------------------
{
#ifdef _WIN32

    char* name = NULL;

    // --- use Windows _tempnam function to get a pointer to an
    //     unused file name that begins with "en"
    name = _tempnam(NULL, "en");
    if (name == NULL) return NULL;

    // --- copy the file name to fname
    if (strlen(name) < MAXFNAME) strncpy(fname, name, MAXFNAME);
    else fname = NULL;

    // --- free the pointer returned by _tempnam
    if (name) free(name);

    // --- for non-Windows systems:
#else
    // --- use system function mkstemp() to create a temporary file name
    int f = -1;
    strcpy(fname, "enXXXXXX");
    f = mkstemp(fname);
    close(f);
    remove(fname);
#endif
    return fname;
}

LRossman · 2019-09-26T15:36:20Z

I just pushed my fixes for issue #528, using a conditional #include unistd.h in project.c (see PR #536). It failed the the gcc-based Travis CI test with a "cannot find unistd.h" error. When I switched to my option of using fdopen() it passed Travis CI.

P.S. I left the unistd.h code in project.c commented out, to be deleted in a future cleanup PR if we decide using fdopen is the way to go..

@ehsan-shafiee

* removing reference to strncpy * Fixing memory problems with test_toolkit Fixes memory leaks and some minor refactoring. * Update test_toolkit.hpp removing crtdbg.h from header * Update CMakeLists.txt Restoring test_net_builder to test_toolkit.exe * Cleaning up include statements adding crtdbg.h * Fixing index error in test * Add more analysis options to the API (issue #425) * Fixed epanet2_enums.h * Eliminates use of temporary linked lists to process Patterns & Curves (issue #449) * Update input2.c * Bug fix for 2Comp and LIFO tank mixing models (issue #448) * Triggering build to update benchmarks * Added new reg tests Updating reference build id * Initial commit list generic linked list * Update test_list.cpp Tests are passing * Update list.h Adding documentation * Fix typo * Fixing bug in head_list * Fixing indentation * Fixed memory leak Fixed memory leak in test_head_list * Clean up and inline comments * Updating file headers * Update list.c Updating in line comments. * Update test_list.cpp * Fixing indent Spaces not tabs * Update list.c Fixing indent * Update test_list.cpp Updating file header to reflect proper attribution * Expanding test Added test where data is a struct * Fixing indent * Work in progress * Reorganized to contain list abstraction * Update list.c * Refactoring head_list and tail_list Simplifying head and tail list. Adding delete_node() to list API. * Update test_list.cpp * Update test_list.cpp Fixing bug on gcc * Fixing bug * Fixing bug on gcc * Update CMakeLists.txt Adding test_list to ctest * Fixes memory leak in EN_addnode() (#455) * Fixing memory leak in EN_addnode() * Separating test_net_builder from test_toolkit Making test_net_builder a standalone test * Removing BOOST_TEST_MAIN * Work in progress * Updating unit tests * Fixing compilation bug on gcc * Work in progress compiles with warnings, definitely not working * Update demand.h * Work in progress Implementing demand_list * Work in progress Creating function for validateing element ID strings * Work in progress Refactoring cstr_copy and adding test * Update cstr_helper.c fixing indentation * Update cstr_helper.c Fixing indentation * Update test_cstrhelper.cpp Fixed mem leak * Adding element id validity checks * Adding element id validity check Adding checks for element set id functions * Fixing build warnings on gcc * Update errror code from 250 to 252 * Work in progress Implementing generic demand pattern lists. Compiles but does not run. * Update demand.c Work in progress * Return object index from EN_addnode and EN_addlink (issue #432) Adds an output argument to EN_addnode and EN_addlink that returns the index of the newly added object. Also refactors the validity check on object ID names. * Fixed compilation errors * Update test_node.cpp * Create test_demand_data.cpp * test demand data passing * Work in progress Fixing problems when demand lists are null * Passing open and close test * get/set demand name are passing * Updated criteria for valid object ID name * Work in progress * Work in progress Working on demand lists * Work in progress Fixing memory leaks Unit tests passing * Cleaning up build on gcc * Cleaning up gcc build * Fixing bug * Working on gcc bug Tests are passing on Appveyor * Update inpfile.c Trying to isolate bug * GCC Bug * Refactored xstrcpy function * Update inpfile.c Testing linux build * Update epanet.c Trying to isolate bug * updating get demand name and write demands Everything passing locally * Update test_project.cpp Isolating bug on gcc * Isolating bug Not writing demand section of input file should eliminate it * Update demand.c Fixing bug in get_category_name when category_name is NULL * Restoring write_demands section in saveinpfile * Update test_demand_data.cpp Adding index to addnode calls. Fixing indent * Update demand.c * Reverted handling of default pattern When creating demands, no pattern is marked with a zero. Then when data is adjusted it gets updated to default. * Update epanet.c Updating EN_getnodevalue() and EN_setnodevalue() to process the primary demand located at the head of the demand list * Update demand.c * Work in progress code cleanup, addressed issue raised in review, and implemented EN_adddemand() * Adding key and search to list * Adding remove node method to generic list * Adding remove demand method to toolkit * Fix bug and test remove demand * Fix problems with setting tank parameters (issue #464 ) * Fixed NULL pointer error, if no label is provided after the rule keyword. * Create Makefile2.bat Co-Authored-By: Demetrios G. Eliades <eldemet@users.noreply.github.com> Co-Authored-By: Elad Salomons <selad@optiwater.com> * Create LICENSE * Fixed NULL pointer error, if no label is provided after the rule keyword. Add NULL guard in freerules function. Use strncat and strncpy to ensure the buffer lengths are adhered to. * For "conditional" do delete a node connected to a link For "conditional" deletion the node is deleted only if all of its links have been explicitly deleted beforehand #473 Co-Authored-By: Lew Rossman <lrossman@outlook.com> * Create CODE_OF_CONDUCT.md * Refactors the API's demand editing functions * Update test_demand.cpp * Update CODE_OF_CONDUCT.md * Update rules.c Fix broken win build script * Updates to doc files * Documentation edits * Update Makefile.bat Updates on the Microsoft SDK 7.1 compilation script to generate runepanet.exe and to use the \include\epanet2.def * Update Makefile2.bat Modified epanet2.exe to runepanet.exe, for consistency. * Delete epanet2.def Deleted the redundant `epanet2.def` file in the WindSDK folder * Minor format change to status report * Removing status reports from CI testing * rm WinSDK folder and update Makefiles Co-Authored-By: Demetrios G. Eliades <eldemet@users.noreply.github.com> * Restored CI testing of status reports * Removes _DEBUG directives from all source files This commit removes the #ifdef _DEBUG statements at the top of all source code files per issue #482. It also updates the doc files to stress that the speedup observed for hydraulic analysis with the MMD node re-ordering method only applies to single period runs. * Fix refactor of types.h * updates authors * updates AUTHORS and generator script * Update run\CMakeLists.txt * add help file win_build.md Co-Authored-By: Elad Salomons <selad@optiwater.com> * move win_build.md to root dir and renaiming to BUILDING.md * Move BuildAndTest.md to the tools directory * Update BUILDING.md * Update BUILDING.md * Update BUILDING.md * Fixes problem with findpattern() function (issue #498) * Change default properties for new pipe created with EN_addlink (issue #500) * Numerous updates to project documentation * Adds tank overflow feature * Updating docs for tank overflow feature * Updating VB include files * Update input3.c * Identifies overflowing tank in Status Report * Update Makefile.bat * Update Makefile2.bat #508 * rethinking the python wrapper (#511) * renames certain function parameter declarations and removes double pointer call from the deleteproject function * deprecates conditonal compilation, removes python-specific headers and function renaming * fixes tests and docs * fixes test * PDA fixes * Minor update to force new CI test * Another minor change to force another CI test * Fixes Overflow and PDA tests not being run * Fix EN_getElseaction and EN_setelseaction Co-Authored-By: Andreas Ashikkis <andreasashikkis@users.noreply.github.com> * Add -MT switch for CMake Windows build * Updates to the docs * Update BUILDING.md * Build script updates * Fixes EN_setlinkvalue bug * fix in EN_deleteLink when pipes are deleted via deletelink it also deletes comment of last link Co-Authored-By: Pavlos Pavlou <pavlou.v.pavlos@ucy.ac.cy> * rm set to null in functions EN_deletenode, EN_deletelink * trial actions config * Update ccpp.yml * welcome to the Actions beta * fixes mkstemp file handle-leaking behavior (#529) * reverts posix include (#533) ... because it is not needed * Fixes bugs in pump and demand head loss gradients * Removed dependence on unistd.h in project.c Travis CI failed because system could not find unistd.h. * getTmpName() and xstrcpy() made safer * Fixed use of strncpy in xstrcpy() * Refactor of hydcoeffs.c Simplifies and unifies how limit on head gradient at low flow is handled. * Update ReleaseNotes2_2.md * Return error if node/link name is too long (#535) * co-authored with @ehsan-shafiee * removes errant slashes * Throws correct error for ID name too long * Revert "Throws correct error for ID name too long" This reverts commit 57b4873. * fixes #534 by bubbling error codes up from add node/link internal functions * fixes tests on Mac at least * fixes improper success code * Error 252 (not 250) returned for ID name too long. From errors.dat: DAT(252,"invalid ID name") * Fixes problems with EN_addnode() (#543) See issue #542 . Also modifies unit test test_node to check that fixup works. * Adds EN_getresultindex function to the API See issue #546 . Also fixes a small bug in project.c. * Adds link vertex get/set functions to the API * Fixes to EN_addlink and EN_deletelink * Updates the docs * Bug fix for EN_setcurve Adjusts params of any pump that uses the curve whose data is modified by EN_setcurve or EN_setcurvevalue (issue #550 ). * Bug fix for EN_getrule Fixes possible seg fault condition in EN_getrule. Also defines EN_MISSING as an API constant since it can be assigned internally to several variables that are retrievable by the API. * Updating the docs * Adds error check to EN_setheadcurveindex See issue #556 . * Update epanet2.pas * Incorrect characterd There was a character ’ instead of ' which created an error when compiling LaTeX. * fixes a crashing issue in freedata (#559) The freedata function used cached values for sizes of certain arrays found in the parser struct. However, now that the network is mutable, those values can become invalid. Relying instead on the actual array lengths prevents freeing unallocated memory, or ignoring cleanup on newly created elements. * Bug fix for valvecheck function See issue #561 * Restored prior update to project.c that got overwritten * Fixed editing errors made to project.c * PDF Guide PDF users' guide for EPANET, and some minor corrections to readme.md to fix some formatting issues. * HTML Users Guide * Fixes a "copy over" bug in input3.c The copying of one input line token over another was causing a compilation error under Clang. With v2.2 this copying is no longer needed so the line of code in question was simply deleted. This commit also deletes the HTML and Latex output generated by running Doxygen that got added from the previous update to dev since they don't really belong in a source code repo. * Correction made to doc files The output-format.dox file was deprecated and not included in the doxyfile so it was deleted. The description of the format of of the Energy Usage section of the binary output in toolkit-files.dox was corrected. * Update ReleaseNotes2_2.md I added the v2.2 contributing authors to the notes. I checked PR's from 2017 and beyond and these were the only names I could find. Please append any one I might have missed. * Fixes problem with re-opening const. HP pumps See latest comments in issue #528. Also, the setlinkflow() function was deleted as it was never called anywhere. * Update README.md (#539) * Update README.md * Update README.md Some section titles were re-named to conform to GitHub guidelines and the OWA info was moved to a CREDITS section. * Update README.md Added link to the Community Forum page. * Replaced OWA copyright with "(see AUTHORS)". * Update AUTHORS Copied format used by the OWA-SWMM project. * Update README.md The Disclaimer section was edited to reflect that there actually is a "collaborative" connection between USEPA and OWA. * updates CI badges * cleanup of readme links and unused files * possessive vs contraction * adding contributor to notes

fixes mkstemp file handle-leaking behavior

9c2d503

samhatchett requested review from eladsal and LRossman September 18, 2019 17:10

samhatchett mentioned this pull request Sep 18, 2019

Temporary files are in the wrong location and clutter the filesystem on non-windows systems #512

Closed

LRossman approved these changes Sep 18, 2019

View reviewed changes

samhatchett merged commit 647abd7 into dev Sep 25, 2019

samhatchett deleted the fix-linux-tempfile branch September 25, 2019 17:24

rjanke20 mentioned this pull request Dec 6, 2019

v2.2 RC #570

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes use of mkstemp and associated file-descriptor-leaking behavior #529

fixes use of mkstemp and associated file-descriptor-leaking behavior #529

fixes use of mkstemp and associated file-descriptor-leaking behavior #529

fixes use of mkstemp and associated file-descriptor-leaking behavior #529

Conversation