Re: [stunnel-users] fix resource leaks and potential null
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
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@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
Checking if errstr is null On Monday, February 4, 2013, Arthur Mesh wrote:
On Mon, Feb 04, 2013 at 02:48:36PM -0500, Brian Wilkins wrote:
Looks like a false positive..checking if a char array is null in that manner is a well known convention.
Sorry, which of the three "defects" are you referring to?
On Tue, Feb 05, 2013 at 05:25:59AM -0500, Brian Wilkins wrote:
Checking if errstr is null
On Mon, Feb 04, 2013 at 02:48:36PM -0500, Brian Wilkins wrote:
Looks like a false positive..checking if a char array is null in that manner is a well known convention.
Actually, I disagree. errstr is a pointer to a character, not a character array. If you take a closer look, you'll see that errstr is initialized conditionally. Whereas its value is checked unconditionally. Now, if you're implying that there is no way it's never initialized before its value is checked, then fine. Otherwise, it seems like a bug. Thanks
No matter..this is the right way to check if a pointer is null in C. http://stackoverflow.com/questions/7970617/how-can-i-check-if-char-variable-... On Tuesday, February 5, 2013, Arthur Mesh wrote:
On Tue, Feb 05, 2013 at 05:25:59AM -0500, Brian Wilkins wrote:
Checking if errstr is null
On Mon, Feb 04, 2013 at 02:48:36PM -0500, Brian Wilkins wrote:
Looks like a false positive..checking if a char array is null in that manner is a well known convention.
Actually, I disagree. errstr is a pointer to a character, not a character array.
If you take a closer look, you'll see that errstr is initialized conditionally. Whereas its value is checked unconditionally. Now, if you're implying that there is no way it's never initialized before its value is checked, then fine. Otherwise, it seems like a bug.
Thanks
On Tue, Feb 05, 2013 at 12:57:47PM -0500, Brian Wilkins wrote:
No matter..this is the right way to check if a pointer is null in C. http://stackoverflow.com/questions/7970617/how-can-i-check-if-char-variable-...
Yes, but you can't check for a value of an uninitialized variable.
It's a variable that points to null, this is a valid check to see if the pointer is null. Coverity is showing a false positive. On Feb 5, 2013 1:10 PM, "Arthur Mesh" <arthurmesh@gmail.com> wrote:
On Tue, Feb 05, 2013 at 12:57:47PM -0500, Brian Wilkins wrote:
No matter..this is the right way to check if a pointer is null in C.
http://stackoverflow.com/questions/7970617/how-can-i-check-if-char-variable-...
Yes, but you can't check for a value of an uninitialized variable.
On Tue, Feb 05, 2013 at 01:41:30PM -0500, Brian Wilkins wrote:
It's a variable that points to null.
Only if you initialize it. It's not initialized during declaration, hence its value is "garbage" from stack. It's only initialized conditionally later on.
this is a valid check to see if the pointer is null. Coverity is showing a false positive.
I don't see any evidence showing that it's guaranteed to be initialized before you read its value in: 2113 if(errstr) { 2114 s_log(LOG_ERR, "Service [%s]: %s", section->servname, 2115 errstr); 2116 return 1; 2117 } In any case. if you're convinced it's a false positive, so be it.
You are completely correct in that regard, but checking if a pointer is null is a different concept. It's not as if the program tried to see if it actually contained a string. On Feb 5, 2013 1:45 PM, "Arthur Mesh" <arthurmesh@gmail.com> wrote:
On Tue, Feb 05, 2013 at 01:41:30PM -0500, Brian Wilkins wrote:
It's a variable that points to null.
Only if you initialize it. It's not initialized during declaration, hence its value is "garbage" from stack. It's only initialized conditionally later on.
this is a valid check to see if the pointer is null. Coverity is showing a false positive.
I don't see any evidence showing that it's guaranteed to be initialized before you read its value in:
2113 if(errstr) { 2114 s_log(LOG_ERR, "Service [%s]: %s", section->servname, 2115 errstr); 2116 return 1; 2117 }
In any case. if you're convinced it's a false positive, so be it.
On Tue, Feb 05, 2013 at 01:52:12PM -0500, Brian Wilkins wrote:
You are completely correct in that regard, but checking if a pointer is null is a different concept. It's not as if the program tried to see if it actually contained a string.
I am not sure why you are bringing up the concept of empty string here. I don't think it's relevant at all. Yes, errstr points to a string, but we don't dereference it check first char is not \0. In any case, looking further in to this defect, I believe it's false positive. The following bit from coverity is bogus: 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 } Note that "At (21): Condition "section", taking false branch" can never be true, since section is guaranteed to be non-NULL (due to section==new_service_options.next, whereas new_service_options.next != NULL). Thoughts? What about the other two UNINIT defects? I assume memory leak defects are pretty obvious. Thanks
Of course if we did dereference it, the program would likely segfault. I have not had a chance to look at the other issues, but I will soon. On Feb 5, 2013 2:31 PM, "Arthur Mesh" <arthurmesh@gmail.com> wrote:
On Tue, Feb 05, 2013 at 01:52:12PM -0500, Brian Wilkins wrote:
You are completely correct in that regard, but checking if a pointer is null is a different concept. It's not as if the program tried to see if it actually contained a string.
I am not sure why you are bringing up the concept of empty string here. I don't think it's relevant at all. Yes, errstr points to a string, but we don't dereference it check first char is not \0.
In any case, looking further in to this defect, I believe it's false positive. The following bit from coverity is bogus:
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 }
Note that "At (21): Condition "section", taking false branch" can never be true, since section is guaranteed to be non-NULL (due to section==new_service_options.next, whereas new_service_options.next != NULL).
Thoughts?
What about the other two UNINIT defects? I assume memory leak defects are pretty obvious.
Thanks
Looking at the addition of the close(fd), that is fine but when main() exits, the file descriptors will effectively be closed. Stylistically, its best to close them when you are done. Up to Michal if he wants to fix that part. On Feb 5, 2013 2:45 PM, "Brian Wilkins" <bwilkins@gmail.com> wrote:
Of course if we did dereference it, the program would likely segfault.
I have not had a chance to look at the other issues, but I will soon. On Feb 5, 2013 2:31 PM, "Arthur Mesh" <arthurmesh@gmail.com> wrote:
On Tue, Feb 05, 2013 at 01:52:12PM -0500, Brian Wilkins wrote:
You are completely correct in that regard, but checking if a pointer is null is a different concept. It's not as if the program tried to see if it actually contained a string.
I am not sure why you are bringing up the concept of empty string here. I don't think it's relevant at all. Yes, errstr points to a string, but we don't dereference it check first char is not \0.
In any case, looking further in to this defect, I believe it's false positive. The following bit from coverity is bogus:
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 }
Note that "At (21): Condition "section", taking false branch" can never be true, since section is guaranteed to be non-NULL (due to section==new_service_options.next, whereas new_service_options.next != NULL).
Thoughts?
What about the other two UNINIT defects? I assume memory leak defects are pretty obvious.
Thanks
I will have to concede my point based on Jochen's reply. Setting it equal to NULL initially would be the guaranteed way that the NULL check would work. However, in all likelihood this would probably never be a problem here. On Tuesday, February 5, 2013, Brian Wilkins wrote:
Looking at the addition of the close(fd), that is fine but when main() exits, the file descriptors will effectively be closed. Stylistically, its best to close them when you are done. Up to Michal if he wants to fix that part. On Feb 5, 2013 2:45 PM, "Brian Wilkins" <bwilkins@gmail.com<javascript:_e({}, 'cvml', 'bwilkins@gmail.com');>> wrote:
Of course if we did dereference it, the program would likely segfault.
I have not had a chance to look at the other issues, but I will soon. On Feb 5, 2013 2:31 PM, "Arthur Mesh" <arthurmesh@gmail.com<javascript:_e({}, 'cvml', 'arthurmesh@gmail.com');>> wrote:
On Tue, Feb 05, 2013 at 01:52:12PM -0500, Brian Wilkins wrote:
You are completely correct in that regard, but checking if a pointer is null is a different concept. It's not as if the program tried to see if it actually contained a string.
I am not sure why you are bringing up the concept of empty string here. I don't think it's relevant at all. Yes, errstr points to a string, but we don't dereference it check first char is not \0.
In any case, looking further in to this defect, I believe it's false positive. The following bit from coverity is bogus:
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 }
Note that "At (21): Condition "section", taking false branch" can never be true, since section is guaranteed to be non-NULL (due to section==new_service_options.next, whereas new_service_options.next != NULL).
Thoughts?
What about the other two UNINIT defects? I assume memory leak defects are pretty obvious.
Thanks
On 05.02.2013 19:44, Arthur Mesh wrote:
On Tue, Feb 05, 2013 at 01:41:30PM -0500, Brian Wilkins wrote:
It's a variable that points to null. Only if you initialize it. It's not initialized during declaration, hence its value is "garbage" from stack.
... unless compiler/library/OS follow one of several modern standards (ANSI C being one of them) that call for memory to be scrubbed upon initial or re-use. Which certainly still isn't a bulletproof vest yet in our day and age. On 05.02.2013 19:52, Brian Wilkins wrote:
You are completely correct in that regard, but checking if a pointer is null is a different concept.
In terms of memory allocation and scrubbing, a pointer is no different from any other type with as many bytes as a memory address needs. In other words, *if* scrubbing fails to happen, you have no better idea of what its contents may be than "a bunch of random bits". Technically, "the" NULL pointer doesn't need to be all zero bits (which necessitates pinpointing when a *numeric* zero value requires to be *explicitly* cast to a pointer type and when not; see, for example, http://www.lysator.liu.se/c/c-faq/c-1.html ), nor does it even need to be *exactly one* possible value (but they are still required to always yield true with "=="; again, ANSI C is among those who say so, in section 3.2.2.3). However, dropping a *completely random* value into a pointer can fail to provide "a NULL pointer" in far more ways than accidentally matching a value that *was*, in fact, returned by malloc(), or happens to be the "&" of some variable; there's no requirement that the content of pointers shall be held against the TLB or whatever source of "legitimate pointer values" to detect cases of "points to unassigned memory, hence to be treated as a NULL pointer". Otherwise, we would have abolished SIGSEGVs years ago - along with three orders of magnitude of the hardware's computing power, I guess. Regards, J. Bern -- *NEU* - NEC IT-Infrastruktur-Produkte im <http://www.linworks-shop.de/>: Server--Storage--Virtualisierung--Management SW--Passion for Performance Jochen Bern, Systemingenieur --- LINworks GmbH <http://www.LINworks.de/> Postfach 100121, 64201 Darmstadt | Robert-Koch-Str. 9, 64331 Weiterstadt PGP (1024D/4096g) FP = D18B 41B1 16C0 11BA 7F8C DCF7 E1D5 FAF4 444E 1C27 Tel. +49 6151 9067-231, Zentr. -0, Fax -299 - Amtsg. Darmstadt HRB 85202 Unternehmenssitz Weiterstadt, Geschäftsführer Metin Dogan, Oliver Michel
Doesn't matter if the variable is used before being assigned a value or not. A variable of this import should always be given a default value at the time of declaration. End of story. Arguing about whether its use is before or after it is assigned a value is just silly. On 2/5/2013 1:41 PM, Brian Wilkins wrote:
It's a variable that points to null, this is a valid check to see if the pointer is null. Coverity is showing a false positive.
On Feb 5, 2013 1:10 PM, "Arthur Mesh" <arthurmesh@gmail.com <mailto:arthurmesh@gmail.com>> wrote:
On Tue, Feb 05, 2013 at 12:57:47PM -0500, Brian Wilkins wrote: > No matter..this is the right way to check if a pointer is null in C. > http://stackoverflow.com/questions/7970617/how-can-i-check-if-char-variable-...
Yes, but you can't check for a value of an uninitialized variable.
_______________________________________________ stunnel-users mailing list stunnel-users@stunnel.org https://www.stunnel.org/cgi-bin/mailman/listinfo/stunnel-users
On Tue, 2013-02-05 14:00:48 -0500, Bing H Bang wrote:
Doesn't matter if the variable is used before being assigned a value or not. A variable of this import should always be given a default value at the time of declaration. End of story.
Initializing local variables with dummy values takes the compiler the chance to identify a code path were no real value is assigned by mistake.
Arguing about whether its use is before or after it is assigned a value is just silly.
As far as I understand the discussion, it is not about whether the variable is used before a value is assigned to it, as this is pretty obviously not the case. It is about whether the variable should be checked for being NULL or checked for pointing to '\0' before passing it to a printf()-like function. Ludolf -- Bihl+Wiedemann GmbH Floßwörthstr. 41 68199 Mannheim, Germany Tel: +49 621 33996-0 Fax: +49 621 3392239 e-mail: lholzheid@bihl-wiedemann.de website: www.bihl-wiedemann.de Sitz der Gesellschaft: Mannheim Geschäftsführer: Jochen Bihl, Bernhard Wiedemann Amtsgericht Mannheim HRB 5796
participants (5)
-
Arthur Mesh -
Bing H Bang -
Brian Wilkins -
Jochen Bern -
Ludolf Holzheid