[stunnel-users] fix resource leaks and potential null

Brian Wilkins bwilkins at gmail.com
Mon Feb 4 20:48:36 CET 2013


Looks like a false positive..checking if a char array is null in that
manner is a well known convention.

On Monday, February 4, 2013, Arthur Mesh wrote:

> dereferences in 4.54
> Reply-To:
> In-Reply-To: <mailman.1.1359889202.25775.stunnel-users at stunnel.org>
>
> On Sun, Feb 03, 2013 at 12:00:02PM +0100,
> stunnel-users-request at stunnel.org wrote:
> > > -    char line_text[CONFLINELEN], *errstr;
> > > +    char line_text[CONFLINELEN], *errstr = NULL;
> > Were you able to identify a case where it's used without
> > initialization?  This is interesting.
> > It would be a good idea to fix it there instead of implementing a
> > workaround here.
>
> Hi,
>
> I will provide the output of coverity run for these defect:
>
> int parse_conf(char *name, CONF_TYPE type) {
> 1997    DISK_FILE *df;
> At (1): Declaring variable "errstr" without initializer.
> 1998    char line_text[CONFLINELEN], *errstr;
> 1999    char config_line[CONFLINELEN], *config_opt, *config_arg;
> 2000    int line_number, i;
> 2001    SERVICE_OPTIONS *section, *new_section;
> 2002    static char *filename=NULL; /* a copy of config file name for
> reloading */
> 2003#ifndef USE_WIN32
> 2004    int fd;
> 2005    char *tmpstr;
> 2006#endif
> 2007
> At (2): Condition "name", taking true branch
> 2008    if(name) /* not reload */
> 2009        filename=str_dup(name);
> 2010
> At (3): Condition "type == 2U", taking true branch
> 2011    s_log(LOG_NOTICE, "Reading configuration from %s %s",
> 2012        type==CONF_FD ? "descriptor" : "file", filename);
> 2013#ifndef USE_WIN32
> At (4): Condition "type == 2U", taking true branch
> 2014    if(type==CONF_FD) { /* file descriptor */
> 2015        fd=strtol(filename, &tmpstr, 10);
> At (5): Condition "tmpstr == filename", taking false branch
> At (6): Condition "*tmpstr", taking false branch
> 2016        if(tmpstr==filename || *tmpstr) { /* not a number */
> 2017            s_log(LOG_ERR, "Invalid file descriptor number");
> 2018            print_syntax();
> 2019            return 1;
> 2020        }
> 2021        df=file_fdopen(fd);
> At (7): Falling through to end of if statement
> 2022    } else
> 2023#endif
> 2024        df=file_open(filename, 0);
> At (8): Condition "!df", taking false branch
> 2025    if(!df) {
> 2026        s_log(LOG_ERR, "Cannot read configuration");
> 2027        if(type!=CONF_RELOAD)
> 2028            print_syntax();
> 2029        return 1;
> 2030    }
> 2031
> 2032    memset(&new_global_options, 0, sizeof(GLOBAL_OPTIONS)); /* reset
> global options */
> 2033    memset(&new_service_options, 0, sizeof(SERVICE_OPTIONS)); /* reset
> local options */
> 2034    new_service_options.next=NULL;
> 2035    section=&new_service_options;
> 2036    parse_global_option(CMD_BEGIN, NULL, NULL);
> 2037    parse_service_option(CMD_BEGIN, section, NULL, NULL);
> At (9): Condition "type != 0U", taking true branch
> 2038    if(type!=CONF_RELOAD) { /* provide defaults for gui.c */
> 2039        memcpy(&global_options, &new_global_options,
> sizeof(GLOBAL_OPTIONS));
> 2040        memcpy(&service_options, &new_service_options,
> sizeof(SERVICE_OPTIONS));
> 2041    }
> 2042
> 2043    line_number=0;
> At (10): Condition "file_getline(df, line_text, 16384) >= 0", taking true
> branch
> At (19): Condition "file_getline(df, line_text, 16384) >= 0", taking false
> branch
> 2044    while(file_getline(df, line_text, CONFLINELEN)>=0) {
> 2045        memcpy(config_line, line_text, CONFLINELEN);
> 2046        ++line_number;
> 2047        config_opt=config_line;
> At (11): Condition "__istype((unsigned char)*config_opt, 16384UL)", taking
> true branch
> At (12): Condition "__istype((unsigned char)*config_opt, 16384UL)", taking
> false branch
> 2048        while(isspace((unsigned char)*config_opt))
> 2049            ++config_opt; /* remove initial whitespaces */
> At (13): Condition "i >= 0", taking true branch
> At (14): Condition "__istype((unsigned char)config_opt[i], 16384UL)",
> taking true branch
> At (15): Condition "i >= 0", taking true branch
> At (16): Condition "__istype((unsigned char)config_opt[i], 16384UL)",
> taking false branch
> 2050        for(i=strlen(config_opt)-1; i>=0 && isspace((unsigned
> char)config_opt[i]); --i)
> 2051            config_opt[i]='\0'; /* remove trailing whitespaces */
> At (17): Condition "config_opt[0] == 0", taking true branch
> 2052        if(config_opt[0]=='\0' || config_opt[0]=='#' ||
> config_opt[0]==';') /* empty or comment */
> At (18): Continuing loop
> 2053            continue;
> 2054        if(config_opt[0]=='[' &&
> config_opt[strlen(config_opt)-1]==']') { /* new section */
> 2055            if(!new_service_options.next) {
> 2056                errstr=parse_global_option(CMD_END, NULL, NULL);
> 2057                if(errstr) {
> 2058                    s_log(LOG_ERR, "Line %d: \"%s\": %s", line_number,
> line_text, errstr);
> 2059                    file_close(df);
> 2060                    return 1;
> 2061                }
> 2062            }
> 2063            ++config_opt;
> 2064            config_opt[strlen(config_opt)-1]='\0';
> 2065            new_section=str_alloc(sizeof(SERVICE_OPTIONS));
> 2066            memcpy(new_section, &new_service_options,
> sizeof(SERVICE_OPTIONS));
> 2067            new_section->servname=str_dup(config_opt);
> 2068            new_section->session=NULL;
> 2069            new_section->next=NULL;
> 2070            section->next=new_section;
> 2071            section=new_section;
> 2072            continue;
> 2073        }
> 2074        config_arg=strchr(config_line, '=');
> 2075        if(!config_arg) {
> 2076            s_log(LOG_ERR, "Line %d: \"%s\": No '=' found",
> line_number, line_text);
> 2077            file_close(df);
> 2078            return 1;
> 2079        }
> 2080        *config_arg++='\0'; /* split into option name and argument
> value */
> 2081        for(i=strlen(config_opt)-1; i>=0 && isspace((unsigned
> char)config_opt[i]); --i)
> 2082            config_opt[i]='\0'; /* remove trailing whitespaces */
> 2083        while(isspace((unsigned char)*config_arg))
> 2084            ++config_arg; /* remove initial whitespaces */
> 2085        errstr=parse_service_option(CMD_EXEC, section, config_opt,
> config_arg);
> 2086        if(!new_service_options.next && errstr==option_not_found)
> 2087            errstr=parse_global_option(CMD_EXEC, config_opt,
> config_arg);
> 2088        if(errstr) {
> 2089            s_log(LOG_ERR, "Line %d: \"%s\": %s", line_number,
> line_text, errstr);
> 2090            file_close(df);
> 2091            return 1;
> 2092        }
> 2093    }
> 2094    file_close(df);
> 2095
> At (20): Condition "new_service_options.next", taking true branch
> 2096    if(new_service_options.next) { /* daemon mode: initialize sections
> */
> At (21): Condition "section", taking false branch
> 2097        for(section=new_service_options.next; section;
> section=section->next) {
> 2098            s_log(LOG_INFO, "Initializing service [%s]",
> section->servname);
> 2099            errstr=parse_service_option(CMD_END, section, NULL, NULL);
> 2100            if(errstr)
> 2101                break;
> 2102        }
> At (22): Falling through to end of if statement
> 2103    } else { /* inetd mode: need to initialize global options */
> 2104        errstr=parse_global_option(CMD_END, NULL, NULL);
> 2105        if(errstr) {
> 2106            s_log(LOG_ERR, "Global options: %s", errstr);
> 2107            return 1;
> 2108        }
> 2109        s_log(LOG_INFO, "Initializing inetd mode configuration");
> 2110        section=&new_service_options;
> 2111        errstr=parse_service_option(CMD_END, section, NULL, NULL);
> 2112    }
> CID 32706: Uninitialized pointer read (UNINIT)At (23): Using uninitialized
> value "errstr".
> 2113    if(errstr) {
> CID 32693: Dereference after null check (FORWARD_NULL) [select defect]
> 2114        s_log(LOG_ERR, "Service [%s]: %s", section->servname, errstr);
> 2115        return 1;
> 2116    }
>
> > > -        s_log(LOG_ERR, "Service [%s]: %s", section->servname, errstr);
> > > +        s_log(LOG_ERR, "Service [%s]: %s", section ?
> section->servname : "",
> > > +              errstr);
> > Again it would be useful to fix the root cause instead of implementing a
> > workaround.
>
>
> Coverity output:
>
> At (1): Condition "name", taking true branch
> 2008    if(name) /* not reload */
> 2009        filename=str_dup(name);
> 2010
> At (2): Condition "type == 2U", taking true branch
> 2011    s_log(LOG_NOTICE, "Reading configuration from %s %s",
> 2012        type==CONF_FD ? "descriptor" : "file", filename);
> 2013#ifndef USE_WIN32
> At (3): Condition "type == 2U", taking true branch
> 2014    if(type==CONF_FD) { /* file descriptor */
> 2015        fd=strtol(filename, &tmpstr, 10);
> At (4): Condition "tmpstr == filename", taking false branch
> At (5): Condition "*tmpstr", taking false branch
> 2016        if(tmpstr==filename || *tmpstr) { /* not a number */
> 2017            s_log(LOG_ERR, "Invalid file descriptor number");
> 2018            print_syntax();
> 2019            return 1;
> 2020        }
> 2021        df=file_fdopen(fd);
> At (6): Falling through to end of if statement
> 2022    } else
> 2023#endif
> 2024        df=file_open(filename, 0);
> At (7): Condition "!df", taking false branch
> 2025    if(!df) {
> 2026        s_log(LOG_ERR, "Cannot read configuration");
> 2027        if(type!=CONF_RELOAD)
> 2028            print_syntax();
> 2029        return 1;
> 2030    }
> 2031
> 2032    memset(&new_global_options, 0, sizeof(GLOBAL_OPTIONS)); /* reset
> global options */
> 2033    memset(&new_service_options, 0, sizeof(SERVICE_OPTIONS)); /* reset
> local options */
> 2034    new_service_options.next=NULL;
> 2035    section=&new_service_options;
> 2036    parse_global_option(CMD_BEGIN, NULL, NULL);
> 2037    parse_service_option(CMD_BEGIN, section, NULL, NULL);
> At (8): Condition "type != 0U", taking true branch
> 2038    if(type!=CONF_RELOAD) { /* provide defaults for gui.c */
> 2039        memcpy(&global_options, &new_global_options,
> sizeof(GLOBAL_OPTIONS));
> 2040        memcpy(&service_options, &new_service_options,
> sizeof(SERVICE_OPTIONS));
> 2041    }
> 2042
> 2043    line_number=0;
> At (9): Condition "file_getline(df, line_text, 16384) >= 0", taking true
> branch
> At (18): Condition "file_getline(df, line_text, 16384) >= 0", taking false
> branch
> 2044    while(file_getline(df, line_text, CONFLINELEN)>=0) {
> 2045        memcpy(config_line, line_text, CONFLINELEN);
> 2046        ++line_number;
> 2047        config_opt=config_line;
> At (10): Condition "__istype((unsigned char)*config_opt, 16384UL)", taking
> true branch
> At (11): Condition "__istype((unsigned char)*config_opt, 16384UL)", taking
> false branch
> 2048        while(isspace((unsigned char)*config_opt))
> 2049            ++config_opt; /* remove initial whitespaces */
> At (12): Condition "i >= 0", taking true branch
> At (13): Condition "__istype((unsigned char)config_opt[i], 16384UL)",
> taking true branch
> At (14): Condition "i >= 0", taking true branch
> At (15): Condition "__istype((unsigned char)config_opt[i], 16384UL)",
> taking false branch
> 2050        for(i=strlen(config_opt)-1; i>=0 && isspace((unsigned
> char)config_opt[i]); --i)
> 2051            config_opt[i]='\0'; /* remove trailing whitespaces */
> At (16): Condition "config_opt[0] == 0", taking true branch
> 2052        if(config_opt[0]=='\0' || config_opt[0]=='#' ||
> config_opt[0]==';') /* empty or comment */
> At (17): Continuing loop
> 2053            continue;
> 2054        if(config_opt[0]=='[' &&
> config_opt[strlen(config_opt)-1]==']') { /* new section */
> 2055            if(!new_service_options.next) {
> 2056                errstr=parse_global_option(CMD_END, NULL, NULL);
> 2057                if(errstr) {
> 2058                    s_log(LOG_ERR, "Line %d: \"%s\": %s", line_number,
> line_text, errstr);
> 2059                    file_close(df);
> 2060                    return 1;
> 2061                }
> 2062            }
> 2063            ++config_opt;
> 2064            config_opt[strlen(config_opt)-1]='\0';
> 2065            new_section=str_alloc(sizeof(SERVICE_OPTIONS));
> 2066            memcpy(new_section, &new_service_options,
> sizeof(SERVICE_OPTIONS));
> 2067            new_section->servname=str_dup(config_opt);
> 2068            new_section->session=NULL;
> 2069            new_section->next=NULL;
> 2070            section->next=new_section;
> 2071            section=new_section;
> 2072            continue;
> 2073        }
> 2074        config_arg=strchr(config_line, '=');
> 2075        if(!config_arg) {
> 2076            s_log(LOG_ERR, "Line %d: \"%s\": No '=' found",
> line_number, line_text);
> 2077            file_close(df);
> 2078            return 1;
> 2079        }
> 2080        *config_arg++='\0'; /* split into option name and argument
> value */
> 2081        for(i=strlen(config_opt)-1; i>=0 && isspace((unsigned
> char)config_opt[i]); --i)
> 2082            config_opt[i]='\0'; /* remove trailing whitespaces */
> 2083        while(isspace((unsigned char)*config_arg))
> 2084            ++config_arg; /* remove initial whitespaces */
> 2085        errstr=parse_service_option(CMD_EXEC, section, config_opt,
> config_arg);
> 2086        if(!new_service_options.next && errstr==option_not_found)
> 2087            errstr=parse_global_option(CMD_EXEC, config_opt,
> config_arg);
> 2088        if(errstr) {
> 2089            s_log(LOG_ERR, "Line %d: \"%s\": %s", line_number,
> line_text, errstr);
> 2090            file_close(df);
> 2091            return 1;
> 2092        }
> 2093    }
> 2094    file_close(df);
> 2095
> At (19): Condition "new_service_options.next", taking true branch
> 2096    if(new_service_options.next) { /* daemon mode: initialize sections
> */
> At (20): Comparing "section" to null implies that "section" might be null.
> At (21): Condition
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.stunnel.org/pipermail/stunnel-users/attachments/20130204/c50f16df/attachment.html>


More information about the stunnel-users mailing list