Re: [Crash-utility] Fix use after free in sial variable lists

Lachlan - thanks for chasing this down. Your fix will work and is safe. The real problem is that we call the help and usage functions for the associated command even if we are doing an unload. We should really only call these during a load. Can you try the patch below against your test case?

--- crash-6.0.0/extensions/sial.c 2011-10-25 10:58:15.000000000 -0700 +++ crash-6.0.0.new/extensions/sial.c 2011-11-29 05:59:54.552190994 -0800 @@ -919,25 +919,26 @@ if(!help) return; snprintf(fname, sizeof(fname), "%s_help", name); if(sial_chkfname(fname, 0)) { - help_str=sial_strdup((char*)(unsigned long)sial_exefunc(fname, 0)); snprintf(fname, sizeof(fname), "%s_usage", name); if(sial_chkfname(fname, 0)) { if(load) { opt_str=sial_strdup((char*)(unsigned long)sial_exefunc(fname, 0)); + snprintf(fname, sizeof(fname), "%s_help", name); + help_str=sial_strdup((char*)(unsigned long)sial_exefunc(fname, 0)); help[0]=sial_strdup(name); help[1]=""; help[2]=sial_strdup(opt_str); help[3]=sial_strdup(help_str); help[4]=0; add_sial_cmd(name, run_callback, help, 0); + sial_free(help_str); + sial_free(opt_str); return; } else rm_sial_cmd(name); } - sial_free(help_str); } free(help); - return; } /* -----Original Message----- From: Dave Anderson [ mailto:anderson@redhat.com ] Sent: Tue 11/29/2011 8:55 AM To: Lachlan McIlroy; Discussion list for crash utility usage, maintenance and development Cc: Luc Chouinard Subject: Re: [Crash-utility] Fix use after free in sial variable lists ----- Original Message ----- > > I encountered a segfault in the sial module when repeatedly loading > and unloading > a sial script. The bug is repeatable and it always segfaults on the > 4th unload. > The bug only triggers if a pathname is specified for the sial > script too (just > doing 'load test.sial, unload test.sial, ...' doesn't trigger the > problem). > > The cause of the problem is that sial_freefile() will free the > memory used by the > static and global variable lists but leaves the stale pointer in > the fdata object. > This stale pointer is then assumed to be allocated later by > sial_inivars(). > > I've included a patch below to NULL out the static and global > variable list > pointers after they are deallocated and this fixes the problem for > me although > I'm not totally sure it's the best way to fix this. Makes perfect sense to me -- unless Luc objects or has a better idea, consider it queued for crash-6.0.1. Thanks, Dave > > Lachlan > > ... > crash> extend /home/lmcilroy/bin/sial.so > Core LINUX_RELEASE == '2.6.18-238.12.1.el5' > < Sial interpreter version 3.0 > > Loading sial commands from > /usr/share/sial/crash:/home/lmcilroy/.sial .... Done. > /home/lmcilroy/bin/sial.so: shared object loaded > crash> load very_long_directory_name/test.sial > crash> unload very_long_directory_name/test.sial > crash> load very_long_directory_name/test.sial > crash> unload very_long_directory_name/test.sial > crash> load very_long_directory_name/test.sial > crash> unload very_long_directory_name/test.sial > crash> load very_long_directory_name/test.sial > crash> unload very_long_directory_name/test.sial > Segmentation fault > > > Program received signal SIGSEGV, Segmentation fault. > sial_inivars (sv=0x2cf4488) at sial_var.c:549 > 549 if(!v->ini && v->dv && v->dv->init) { > Missing separate debuginfos, use: debuginfo-install > crash-4.0.9-2.fc12.x86_64 > (gdb) bt > #0 sial_inivars (sv=0x2cf4488) at sial_var.c:549 > #1 0x00007fffed6b1d24 in sial_addsvs (type=1, sv=<value optimized > out>) at sial_var.c:821 > #2 0x00007fffed69f07d in sial_execmcfunc (f=<value optimized out>, > vp=<value optimized out>) at sial_func.c:900 > #3 0x00007fffed6a026b in sial_exefunc (fname=0x7fffffffdc90 > "cciss_help", vp=0x0) at sial_func.c:968 > #4 0x00007fffed69cac2 in reg_callback (name=0x2c30538 "cciss", > load=0) at sial.c:922 > #5 0x00007fffed69f72f in sial_docallback (fd=0x2f2ed58) at > sial_func.c:264 > #6 sial_freefile (fd=0x2f2ed58) at sial_func.c:288 > #7 0x00007fffed69fe2a in sial_deletefile (name=0x7885a28 > "very_long_directory_name/test.sial") at sial_func.c:314 > #8 0x00007fffed6a5ce6 in sial_loadunload (load=0, name=<value > optimized out>, silent=0) at sial_api.c:1289 > #9 0x00007fffed69c77d in unload_cmd () at sial.c:775 > #10 0x000000000045d334 in exec_command () at main.c:740 > #11 0x000000000045d57a in main_loop () at main.c:699 > #12 0x0000000000554d19 in captured_command_loop (data=<value > optimized out>) at ./main.c:228 > #13 0x0000000000552feb in catch_errors (func=<value optimized out>, > func_args=<value optimized out>, errstring=<value optimized out>, > mask=<value optimized out>) at exceptions.c:531 > #14 0x0000000000554a26 in captured_main (data=<value optimized > out>) > at ./main.c:958 > #15 0x0000000000552feb in catch_errors (func=<value optimized out>, > func_args=<value optimized out>, errstring=<value optimized out>, > mask=<value optimized out>) at exceptions.c:531 > #16 0x0000000000553be4 in gdb_main (args=0x2cf4488) at ./main.c:973 > #17 0x0000000000553c1e in gdb_main_entry (argc=<value optimized > out>, > argv=<value optimized out>) at ./main.c:993 > #18 0x000000000045e0df in main (argc=<value optimized out>, > argv=<value optimized out>) at main.c:603 > (gdb) p sv > $1 = (var_t *) 0x2cf4488 > (gdb) p sv->next > $2 = (struct var_s *) 0x7463657269645f67 > > 0x7463657269645f67 = "g-direct" which is part of the sial pathname > (with the > underscore converted) so the memory has been reallocated. > > > diff -up crash-6.0.0/extensions/libsial/sial_func.c.orig > crash-6.0.0/extensions/libsial/sial_func.c > --- crash-6.0.0/extensions/libsial/sial_func.c.orig 2011-11-29 > 13:09:43.985631958 +1100 > +++ crash-6.0.0/extensions/libsial/sial_func.c 2011-11-29 > 13:10:31.930603477 +1100 > @@ -280,8 +280,14 @@ sial_freefile(fdata *fd) > } > > /* free the associated static and global variables */ > - if(fd->fsvs) sial_freesvs(fd->fsvs); > - if(fd->fgvs) sial_freesvs(fd->fgvs); > + if(fd->fsvs) { > + sial_freesvs(fd->fsvs); > + fd->fsvs = NULL; > + } > + if(fd->fgvs) { > + sial_freesvs(fd->fgvs); > + fd->fgvs = NULL; > + } > > /* free all function nodes */ > // let debugger know ... > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility >