Re: [Toybox] [PATCH] tar: fix heap buffer overrun.
On Thu, Oct 15, 2020 at 9:22 PM Rob Landley wrote: > > > > On 10/15/20 7:45 PM, enh wrote: > > On Thu, Oct 15, 2020 at 5:21 PM Rob Landley wrote: > >> > >> On 10/14/20 3:21 PM, enh wrote: > >>> i've sent a new fix that just touches dirtree_path() so that it always > >>> honors the size request again. > >> > >> Applied, and then cosmetically fiddled with because I do that. > >> > but I leave for the airport to fly back to Japan in 2 hours. (Part of the > reason I've been so distracted lately, it's not JUST focusing on sh.c. :) > > > Caught by ASan. > > Operating on what path? > >>> > >>> the new patch's commit message makes it clearer that you can reproduce > >>> this with the existing tar tests, as long as you `export ASAN=1`. > >>> (would we need extra docker dependencies, or should we just turn that > >>> on for the github CI?) > >> > >> No idea. > > > > yeah, i was hoping our github CI expert would chime in :-) > > > >> I mentioned the ndk not working for this becuase of the need to build > >> --static > >> to run anything on a system that doesn't have bionic installed in /lib, > >> and asan > >> not working --static. > >> > >> I can install llvm 7 through the devuan apt-get (may 2019), but 11 just > >> shipped > >> and they don't have debian binaries, just ubuntu. MIGHT work? Not looking > >> forward to trying to build that from source, it's really brittle last I > >> checked... Ah, maybe I can follow: > >> > >> http://www.linuxfromscratch.org/blfs/view/svn/general/llvm.html > >> > >> Um... is ninja part of cmake now? It lists cmake as a dependency but does > >> not > >> list ninja? Did they add ninja to the 10.x base? > >> > >> http://www.linuxfromscratch.org/lfs/view/stable/chapter08/ninja.html > >> > >> yes they did. Lovely... > >> > >> Anyway, question: is llvm 7 likely to be enough, or should I try compiling > >> llvm > >> from source to poke at this asan stuff? > > > > debian testing seems to have llvm 9 atm, and that's what i used, but, > > yes, asan's been stable for a while now so i'd expect 7 should be > > fine. > > "After this operation, 562 MB of additional disk space will be used." Sigh. On > aboriginal linux I had a gcc install in 14 megs and 11 of that was > /usr/include. > Why did they choose to write this in c++ again? remember clang is always a cross-compiler, so one clang == lots of gccs. > So I installed llvm-7 and afterwards did not have either a "clang" or an > "llvm" > in the $PATH. So I installed clang-7 and still don't. I think I'm washing my > hands of debian's packaging of llvm because it DOESN'T WORK. (I now very > vaguely > recall trying this before and uninstalling it again for this reason.) works for me on debian: ~$ dpkg -S `which clang` clang: /usr/bin/clang ~$ update-alternatives --list cc /usr/bin/clang /usr/bin/gcc > I need to poke at LFS 10 anyway. I've mostly got mkroot back together and > booting again with toysh. Still need function() support and to finish > implementing the "source" command, and get the tests passing and turn > shtest.txt > into more tests, but it's almost ready to start throwing real stuff at? What > do > I need... > > Nope, do_math() is still a stub so no $(( )) yet. And I should probably do [[ > ]] > because some build script is likely to use that. And that TODO about \escapes > in > backquotes is probably gonna bite. (Yeah I still need to do array variable > support but I can wait for something to break needing it, same for shopt > nocasematch and so on.) Signal handling at least needs to be stubbed in so > "trap" isn't an unknown command, but them I've got a chunk of job control > pluming implemented already. I don't strictly need to do command line editing > and history before trying to build LFS with it but you REALLY notice it's > missing using it interactively... > > But... huh. It just went over 3500 lines and I'm sad about that, and yet it's > actually most of the way there? Ish? Sort of? Not THIS release, and plenty of > polishing stuff still to do, plus the PILE of bugs any real load is bound to > squeeze out... > > Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] tar: fix heap buffer overrun.
On 10/15/20 7:45 PM, enh wrote: > On Thu, Oct 15, 2020 at 5:21 PM Rob Landley wrote: >> >> On 10/14/20 3:21 PM, enh wrote: >>> i've sent a new fix that just touches dirtree_path() so that it always >>> honors the size request again. >> >> Applied, and then cosmetically fiddled with because I do that. >> but I leave for the airport to fly back to Japan in 2 hours. (Part of the reason I've been so distracted lately, it's not JUST focusing on sh.c. :) > Caught by ASan. Operating on what path? >>> >>> the new patch's commit message makes it clearer that you can reproduce >>> this with the existing tar tests, as long as you `export ASAN=1`. >>> (would we need extra docker dependencies, or should we just turn that >>> on for the github CI?) >> >> No idea. > > yeah, i was hoping our github CI expert would chime in :-) > >> I mentioned the ndk not working for this becuase of the need to build >> --static >> to run anything on a system that doesn't have bionic installed in /lib, and >> asan >> not working --static. >> >> I can install llvm 7 through the devuan apt-get (may 2019), but 11 just >> shipped >> and they don't have debian binaries, just ubuntu. MIGHT work? Not looking >> forward to trying to build that from source, it's really brittle last I >> checked... Ah, maybe I can follow: >> >> http://www.linuxfromscratch.org/blfs/view/svn/general/llvm.html >> >> Um... is ninja part of cmake now? It lists cmake as a dependency but does not >> list ninja? Did they add ninja to the 10.x base? >> >> http://www.linuxfromscratch.org/lfs/view/stable/chapter08/ninja.html >> >> yes they did. Lovely... >> >> Anyway, question: is llvm 7 likely to be enough, or should I try compiling >> llvm >> from source to poke at this asan stuff? > > debian testing seems to have llvm 9 atm, and that's what i used, but, > yes, asan's been stable for a while now so i'd expect 7 should be > fine. "After this operation, 562 MB of additional disk space will be used." Sigh. On aboriginal linux I had a gcc install in 14 megs and 11 of that was /usr/include. Why did they choose to write this in c++ again? So I installed llvm-7 and afterwards did not have either a "clang" or an "llvm" in the $PATH. So I installed clang-7 and still don't. I think I'm washing my hands of debian's packaging of llvm because it DOESN'T WORK. (I now very vaguely recall trying this before and uninstalling it again for this reason.) I need to poke at LFS 10 anyway. I've mostly got mkroot back together and booting again with toysh. Still need function() support and to finish implementing the "source" command, and get the tests passing and turn shtest.txt into more tests, but it's almost ready to start throwing real stuff at? What do I need... Nope, do_math() is still a stub so no $(( )) yet. And I should probably do [[ ]] because some build script is likely to use that. And that TODO about \escapes in backquotes is probably gonna bite. (Yeah I still need to do array variable support but I can wait for something to break needing it, same for shopt nocasematch and so on.) Signal handling at least needs to be stubbed in so "trap" isn't an unknown command, but them I've got a chunk of job control pluming implemented already. I don't strictly need to do command line editing and history before trying to build LFS with it but you REALLY notice it's missing using it interactively... But... huh. It just went over 3500 lines and I'm sad about that, and yet it's actually most of the way there? Ish? Sort of? Not THIS release, and plenty of polishing stuff still to do, plus the PILE of bugs any real load is bound to squeeze out... Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] tar: fix heap buffer overrun.
On Thu, Oct 15, 2020 at 5:21 PM Rob Landley wrote: > > On 10/14/20 3:21 PM, enh wrote: > > i've sent a new fix that just touches dirtree_path() so that it always > > honors the size request again. > > Applied, and then cosmetically fiddled with because I do that. > > >> but I leave for the airport to fly back to Japan in 2 hours. (Part of the > >> reason I've been so distracted lately, it's not JUST focusing on sh.c. :) > >> > >>> Caught by ASan. > >> > >> Operating on what path? > > > > the new patch's commit message makes it clearer that you can reproduce > > this with the existing tar tests, as long as you `export ASAN=1`. > > (would we need extra docker dependencies, or should we just turn that > > on for the github CI?) > > No idea. yeah, i was hoping our github CI expert would chime in :-) > I mentioned the ndk not working for this becuase of the need to build --static > to run anything on a system that doesn't have bionic installed in /lib, and > asan > not working --static. > > I can install llvm 7 through the devuan apt-get (may 2019), but 11 just > shipped > and they don't have debian binaries, just ubuntu. MIGHT work? Not looking > forward to trying to build that from source, it's really brittle last I > checked... Ah, maybe I can follow: > > http://www.linuxfromscratch.org/blfs/view/svn/general/llvm.html > > Um... is ninja part of cmake now? It lists cmake as a dependency but does not > list ninja? Did they add ninja to the 10.x base? > > http://www.linuxfromscratch.org/lfs/view/stable/chapter08/ninja.html > > yes they did. Lovely... > > Anyway, question: is llvm 7 likely to be enough, or should I try compiling > llvm > from source to poke at this asan stuff? debian testing seems to have llvm 9 atm, and that's what i used, but, yes, asan's been stable for a while now so i'd expect 7 should be fine. > Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] tar: fix heap buffer overrun.
On 10/14/20 3:21 PM, enh wrote: > i've sent a new fix that just touches dirtree_path() so that it always > honors the size request again. Applied, and then cosmetically fiddled with because I do that. >> but I leave for the airport to fly back to Japan in 2 hours. (Part of the >> reason I've been so distracted lately, it's not JUST focusing on sh.c. :) >> >>> Caught by ASan. >> >> Operating on what path? > > the new patch's commit message makes it clearer that you can reproduce > this with the existing tar tests, as long as you `export ASAN=1`. > (would we need extra docker dependencies, or should we just turn that > on for the github CI?) No idea. I mentioned the ndk not working for this becuase of the need to build --static to run anything on a system that doesn't have bionic installed in /lib, and asan not working --static. I can install llvm 7 through the devuan apt-get (may 2019), but 11 just shipped and they don't have debian binaries, just ubuntu. MIGHT work? Not looking forward to trying to build that from source, it's really brittle last I checked... Ah, maybe I can follow: http://www.linuxfromscratch.org/blfs/view/svn/general/llvm.html Um... is ninja part of cmake now? It lists cmake as a dependency but does not list ninja? Did they add ninja to the 10.x base? http://www.linuxfromscratch.org/lfs/view/stable/chapter08/ninja.html yes they did. Lovely... Anyway, question: is llvm 7 likely to be enough, or should I try compiling llvm from source to poke at this asan stuff? Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] tar: fix heap buffer overrun.
On Wed, Oct 14, 2020 at 1:56 AM Rob Landley wrote: > > On 10/13/20 4:19 PM, enh via Toybox wrote: > > tar was assuming the old behavior of dirtree_path() where there was > > always a spare byte free at the end. > > It's not the old behavior, tar.c is doing: > > i = 1; > name = hname = dirtree_path(node, ); > ... > // Consume the 1 extra byte alocated in dirtree_path() > if (S_ISDIR(st->st_mode) && name[i-1] != '/') strcat(name, "/"); > > The "1 extra byte" is because we fed i = 1 into for dirtree_path(), which > should result directly in extra space at the end because that function does: > > ll = len = plen ? *plen : 0; > for (nn = node; nn; nn = nn->parent) > if ((ii = strlen(nn->name))) len += ii+1-(nn->name[ii-1]=='/'); > if (plen) *plen = len; > path = xmalloc(len)+len-ll; > > Starting len is value fed in, length of each non-empty node is added in minus > trailing slashes we'd remove, and then it's saved back out and we allocate the > right amount but back up the initial *plen amount from the end (saved in ll > but > honestly I could just transpose those last two lines and not need the temp) so > the new "fill it in backwards" logic winds up at the start of the string and > it's just empty space. > > > Since removing that seems to have > > been an intentional change to dirtree_path(), change the caller to > > resize the string itself. > > I'd rather get the allocation right in the first place (do we need i = 2, if > so > why?), no, the i=1 was right, i just hadn't noticed that the int* was an "inout" parameter. i've sent a new fix that just touches dirtree_path() so that it always honors the size request again. > but I leave for the airport to fly back to Japan in 2 hours. (Part of the > reason I've been so distracted lately, it's not JUST focusing on sh.c. :) > > > Caught by ASan. > > Operating on what path? the new patch's commit message makes it clearer that you can reproduce this with the existing tar tests, as long as you `export ASAN=1`. (would we need extra docker dependencies, or should we just turn that on for the github CI?) > Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
Re: [Toybox] [PATCH] tar: fix heap buffer overrun.
On 10/13/20 4:19 PM, enh via Toybox wrote: > tar was assuming the old behavior of dirtree_path() where there was > always a spare byte free at the end. It's not the old behavior, tar.c is doing: i = 1; name = hname = dirtree_path(node, ); ... // Consume the 1 extra byte alocated in dirtree_path() if (S_ISDIR(st->st_mode) && name[i-1] != '/') strcat(name, "/"); The "1 extra byte" is because we fed i = 1 into for dirtree_path(), which should result directly in extra space at the end because that function does: ll = len = plen ? *plen : 0; for (nn = node; nn; nn = nn->parent) if ((ii = strlen(nn->name))) len += ii+1-(nn->name[ii-1]=='/'); if (plen) *plen = len; path = xmalloc(len)+len-ll; Starting len is value fed in, length of each non-empty node is added in minus trailing slashes we'd remove, and then it's saved back out and we allocate the right amount but back up the initial *plen amount from the end (saved in ll but honestly I could just transpose those last two lines and not need the temp) so the new "fill it in backwards" logic winds up at the start of the string and it's just empty space. > Since removing that seems to have > been an intentional change to dirtree_path(), change the caller to > resize the string itself. I'd rather get the allocation right in the first place (do we need i = 2, if so why?), but I leave for the airport to fly back to Japan in 2 hours. (Part of the reason I've been so distracted lately, it's not JUST focusing on sh.c. :) > Caught by ASan. Operating on what path? Rob ___ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net
