OpenWaterAnalytics / EPANET

The Water Distribution System Hydraulic and Water Quality Analysis Toolkit
MIT License
273 stars 203 forks source link

getclosedlink uses recursion that can lead to a stackoverflow #737

Closed lbutler closed 10 months ago

lbutler commented 1 year ago

The function getclosedlink in report.c uses recursion to find closed links when reporting on disconnections.

In very large networks, it’s possible for the recursion to exhaust the memory on the call stack which then causes EPANET to crash.

This issue was first found in epanet-js because of the combination of the higher overhead in running functions and smaller stack limit. I was able to cause the same issue in owa-epanet with a slightly contrived example using 150,000 disconnections.

I’ve attached an INP which can be used with runepanet to test the bug.

image

The INP was created with the code in this GIST: https://gist.github.com/lbutler/48783e61125ad4adf818fa24e8528811

If a loop is used instead of recursion, EPANET will not crash with very large disconnections. I have created a branch with the changes here:

https://github.com/OpenWaterAnalytics/EPANET/compare/dev...lbutler:EPANET:remove-getclosedlink-recursion

epanet_disconnections.inp.zip

LRossman commented 11 months ago

@lbutler I modified your new getclosedlinkfunction to have the already available array named nodelistin the calling routine be used for the stack in getclosedlinkthereby saving an extra memory allocation. The signature for getclosedlink would be:

static void getclosedlink((Project *, int, char *, int *);

and it's called in the disconnected() function as:

getclosedlink(pr, j, marked, nodelist);

The opening lines of the function look as follows:

void getclosedlink(Project *pr, int i, char *marked, int *stack)
/*
**----------------------------------------------------------------
**   Input:   i = junction index
**            marked[] = marks nodes already examined
**            stack[] = stack to hold nodes to examine
**   Output:  None.
**   Purpose: Determines if a closed link connects to junction i.
**----------------------------------------------------------------
*/
    Network *net = &pr->network;

    int j, k;
    Padjlist alink;

    int top = 0;

    // Mark the current junction as examined and push onto stack

while the rest of your function remains unchanged (except that the free(stack) line was removed).

lbutler commented 10 months ago

Thanks for your suggested changes @LRossman, I've updated my code and create a pull request