Re: [stunnel-users] fix resource leaks and potential null

4 Feb 2013


      dereferences in 4.54
Reply-To: 
In-Reply-To: mailman.1.1359889202.25775.stunnel-users@stunnel.org
On Sun, Feb 03, 2013 at 12:00:02PM +0100, stunnel-users-request@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 "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) [select defect]
At (23): Condition "errstr", taking true branch
2113    if(errstr) {
CID 32693: Dereference after null check (FORWARD_NULL)At (24): Dereferencing null pointer "section".
2114        s_log(LOG_ERR, "Service [%s]: %s", section->servname, errstr);
2115        return 1;
2116    }
...
...

   if(!strcasecmp(fl->name, string)) {


   if(string && !strcasecmp(fl->name, string)) {


Could you give an example parameter where "string" may be NULL here?
2238/* facilities only make sense on Unix */
2239#if !defined (USE_WIN32) && !defined (__vms)
At (1): Condition "strchr(string, 46)", taking true branch
2240    if(strchr(string, '.')) { /* we have a facility specified */
2241        new_global_options.facility=-1;
2242        string=strtok(arg_copy, "."); /* break it up */
2243
At (2): Condition "fl->name", taking true branch
2244        for(fl=facilities; fl->name; ++fl) {
At (3): Condition "!strcasecmp(fl->name, string)", taking true branch
2245            if(!strcasecmp(fl->name, string)) {
2246                new_global_options.facility=fl->value;
At (4): Breaking from loop
2247                break;
2248            }
2249        }
At (5): Condition "new_global_options.facility == -1", taking false branch
2250        if(new_global_options.facility==-1)
2251            return 1; /* FAILED */
2252        string=strtok(NULL, ".");    /* set to the remainder */
2253    }
2254#endif /* USE_WIN32, __vms */
2255
2256    /* time to check the syslog level */
At (6): Comparing "string" to null implies that "string" might be null.
At (7): Condition "string", taking false branch
2257    if(string && strlen(string)==1 && *string>='0' && *string<='7') {
2258        new_global_options.debug_level=*string-'0';
2259        return 0; /* OK */
2260    }
2261    new_global_options.debug_level=8;    /* illegal level */
At (8): Condition "fl->name", taking true branch
2262    for(fl=levels; fl->name; ++fl) {
CID 11183: Dereference after null check (FORWARD_NULL)At (9): Passing null pointer "string" to function "strcasecmp(char const *, char const *)", which dereferences it.
2263        if(!strcasecmp(fl->name, string)) {
2264            new_global_options.debug_level=fl->value;
2265            break;
2266        }
2267    }
2268    if(new_global_options.debug_level==8)
2269        return 1; /* FAILED */
2270    return 0; /* OK */
2271}
Let me know if more info is required.
Thanks