Nmap Development mailing list archives
Re: Looking at nse_pcrelib.cc file using clang static file analyzer.
From: David Fifield <david () bamsoftware com>
Date: Tue, 5 Jun 2012 14:08:11 -0700
On Thu, May 31, 2012 at 04:12:31PM -0400, James Rogers wrote:
Examining the Clang output in more detail. Noticing a few issues which could be errors. Logic error Dereference of null pointer nse_pcrelib.cc 234 Logic error Dereference of null pointer nse_pcrelib.cc 272 This does look like an error that could cause a crash. On line 230 ud is passed as a reference into Lpcre_getargs() to be assigned a value. This appears that it can error out and return a NULL in the function, but this is never checked on return, and ud->ncapt is dereferenced in that case.
I'm not sure about this one. The only dereferences on line 234 are of the variable ud. Lpcre_getargs checks if ud is NULL, which is probably where Clang gets the idea that ud can be NULL, but in that case Lpcre_getargs calls luaL_argerror and doesn't return. It's possible that Clang doesn't recognize luaL_argerror as a function that can do a non-local return. If you replace luaL_argerror with exit, does Clang still warn?
Memory Error Memory leak nse_pcrelib.cc 143 This doesn't appear too bad, because nmap doesn't run very long. Memory leak: 106 static int Lpcre_comp(lua_State *L) 107 { 108 char buf[256]; 109 const char *error; 110 int erroffset; 111 pcre2 *ud; 112 char *pattern = strdup(luaL_checkstring(L, 1)); 1 - Memory is allocated 113 int cflags = luaL_optint(L, 2, 0); 114 const unsigned char *tables = NULL; 115 116 if(lua_gettop(L) > 2 && !lua_isnil(L, 3)) 2 - Taking false branch 117 tables = Lpcre_maketables(L, 3); 118 if(tables == NULL) 3 - Taking true branch 119 luaL_error(L, "PCRE compilation failed"); 120 121 ud = (pcre2*)lua_newuserdata(L, sizeof(pcre2)); 122 luaL_getmetatable(L, pcre_handle); 123 (void)lua_setmetatable(L, -2); 124 ud->match = NULL; 125 ud->extra = NULL; 126 ud->tables = tables; /* keep this for eventual freeing */ 127 128 ud->pr = pcre_compile(pattern, cflags, &error, &erroffset, tables); 129 if(!ud->pr) { 4 - Taking false branch 130 (void)Snprintf(buf, 255, "%s (pattern offset: %d)", error, erroffset+1); 131 /* show offset 1-based as it's common in Lua */ 132 luaL_error(L, buf); 133 } 134 135 ud->extra = pcre_study(ud->pr, 0, &error); 136 if(error) luaL_error(L, error); 5 - Taking false branch 137 138 pcre_fullinfo(ud->pr, ud->extra, PCRE_INFO_CAPTURECOUNT, &ud->ncapt); 139 /* need (2 ints per capture, plus one for substring match) * 3/2 */ 140 ud->match = (int *) safe_malloc((ud->ncapt + 1) * 3 * sizeof(int)); 141 142 return 1; 143 } 6 - Memory is never released; potential leak of memory pointed to by 'pattern'
This looks like a real bug to me. I don't see why pattern is being dynamically allocated at all. It's only passed to pcre_compile (which doesn't take ownership) and never freed. I'm making a commit to keep it a stack variable. David Fifield _______________________________________________ Sent through the nmap-dev mailing list http://cgi.insecure.org/mailman/listinfo/nmap-dev Archived at http://seclists.org/nmap-dev/
Current thread:
- Looking at nse_pcrelib.cc file using clang static file analyzer. James Rogers (May 31)
- Re: Looking at nse_pcrelib.cc file using clang static file analyzer. David Fifield (Jun 05)