Re: Looking at nse_pcrelib.cc file using clang static file analyzer.

[David Fifield <david () bamsoftware com>]

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/