lurcher / unixODBC

The unixODBC Project goals are to develop and promote unixODBC to be the definitive standard for ODBC on non MS Windows platforms.
GNU Lesser General Public License v2.1
102 stars 52 forks source link

fix potential leak found by coverity #198

Closed chipitsine closed 1 week ago

chipitsine commented 3 weeks ago
*** CID 446774:  Resource leaks  (RESOURCE_LEAK)
/DriverManager/SQLDriverConnect.c: 469 in __get_pair() 463
464         __get_attr( cp, &keyword, &value );
465         if ( keyword )
466         {
467             con_p = malloc( sizeof( *con_p ));
468             if ( !con_p )
>>>     CID 446774:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "value" going out of scope leaks the storage it points to.
469                 return NULL;
470             con_p -> keyword = keyword;
471             con_p -> attribute = value;
472             return con_p;
473         }
474         else

*** CID 446773:  Resource leaks  (RESOURCE_LEAK)
/DriverManager/SQLDriverConnect.c: 469 in __get_pair() 463
464         __get_attr( cp, &keyword, &value );
465         if ( keyword )
466         {
467             con_p = malloc( sizeof( *con_p ));
468             if ( !con_p )
>>>     CID 446773:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "keyword" going out of scope leaks the storage it points to.
469                 return NULL;
470             con_p -> keyword = keyword;
471             con_p -> attribute = value;
472             return con_p;
473         }
474         else
chipitsine commented 3 weeks ago

from my view there's bigger problem when local variables are returned out of a function. I wonder why coverity does not worry about it, I plan to have look at it later

lurcher commented 3 weeks ago

On 27/10/2024 18:06, Ilya Shipitsin wrote:

from my view there's bigger problem when local variables are returned out of a function. I wonder why coverity does not worry about it, I plan to have look at it later

It would worry me as well if that was what was happening. I think I trust coverity less and less as time goes on.

chipitsine commented 3 weeks ago

the following is suspicious

image

but that code is there for ~15 years. If there were a problem, it must appear somehow. What makes me think my assumption (about returning local variables|) is wrong

chipitsine commented 1 week ago

@lurcher , can we merge it since it addresses leaks ?

lurcher commented 1 week ago

I don't understand where the leak is. the local variables you mention, are pointers, they are set to point to allocated memory in the call to __get_attr, then the pointed to, allocated memory is returned in the returned allocated structure. Just where does the problem occur?

chipitsine commented 1 week ago

if NULL is returned, so con_p is leaked

lurcher commented 1 week ago

Ahh, ok, I can see that. Will fix that.