Home page logo
/

nmap-dev logo 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/


  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]