[Lldb-commits] [lldb] r235991 - Replace sprintf with snprintf to avoid a crash.

[Ilia K]

It's interesting. Need to check lldb for other places where it can affect.

As I understood the general rule is to cast value to type (that it should
be printed as) before passing to printf.

Thanks,
Ilia
On Apr 28, 2015 6:47 PM, "Pavel Labath" <labath at google.com> wrote:

> Interesting.. I learned something new today... :)
>
>
>
> On 28 April 2015 at 16:40, Abid, Hafiz <Hafiz_Abid at mentor.com> wrote:
> > PRIx8 is defined as "x" in /usr/include/inttypes.h. This is Ubuntu 14.04.
> >
> > #include <stdio.h>
> >  #include <string.h>
> >  #include <inttypes.h>
> > int main()
> > {
> >   char cUnescapedChar = 0xeb;
> >   const int old_size = sizeof("\\xXX");
> >   char strEscapedChar[old_size];
> >   ::sprintf(strEscapedChar, "\\x%02" PRIx8, cUnescapedChar);
> >   int new_size = strlen(strEscapedChar);
> >   printf("old:%d  new:%d\n", old_size, new_size);
> >   printf("%s\n", strEscapedChar);
> >   return 0;
> > }
> >
> > gives this output on my system (with g++  4.8.2).
> > old:5  new:10
> > \xffffffeb
> > *** stack smashing detected ***: ./test terminated
> > Aborted (core dumped)
> >
> > Casting the cUnescapedChar to unsigned char also fixes the crash.
> >
> > Regards,
> > Abid
> >
> >> -----Original Message-----
> >> From: Pavel Labath [mailto:labath at google.com]
> >> Sent: 28 April 2015 16:06
> >> To: Abid, Hafiz
> >> Cc: Ilia K; lldb-commits at cs.uiuc.edu
> >> Subject: Re: [Lldb-commits] [lldb] r235991 - Replace sprintf with
> snprintf to
> >> avoid a crash.
> >>
> >> I find this surprising as well...
> >>
> >> what is PRIx8 defined to on your system? Normally, it should be
> something
> >> like "hhx" as Ilia suggested. Here, "hh" should cast the value to char
> and "x"
> >> to unsigned, so there should be no possibility of overflow.
> >>
> >> Unless I am misunderstanding something...
> >>
> >> cheers,
> >> pl
> >>
> >>
> >> On 28 April 2015 at 15:55, Abid, Hafiz <Hafiz_Abid at mentor.com> wrote:
> >> > Well, it was happening :-(. It is \\x%02x” and cUnescapedChar is not
> >> > unsigned.
> >> >
> >> > So when it had value with MSB 1, then it overflew the buffer.
> >> >
> >> >
> >> >
> >> > There are other problems too with printing the 1-byte register.
> >> > Current code
> >> >
> >> > tries to print it as a character which is probably not the right
> >> > thing. But that
> >> >
> >> > is a separate fix.
> >> >
> >> >
> >> >
> >> > Regards,
> >> >
> >> > Abid
> >> >
> >> >
> >> >
> >> > From: Ilia K [mailto:ki.stfu at gmail.com]
> >> > Sent: 28 April 2015 15:26
> >> > To: Abid, Hafiz
> >> > Cc: lldb-commits at cs.uiuc.edu
> >> > Subject: Re: [Lldb-commits] [lldb] r235991 - Replace sprintf with
> >> > snprintf to avoid a crash.
> >> >
> >> >
> >> >
> >> > Hello Abid,
> >> >
> >> >
> >> >
> >> > I thought it never can happen because cUnescapedChar is less than 255
> >> > and "\\x%02hhu" prints something like \x12. Am I wrong?
> >> >
> >> >
> >> >
> >> > Thanks,
> >> >
> >> > Ilia
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > On Tue, Apr 28, 2015 at 5:16 PM, Hafiz Abid Qadeer
> >> > <hafiz_abid at mentor.com>
> >> > wrote:
> >> >
> >> > Author: abidh
> >> > Date: Tue Apr 28 09:16:00 2015
> >> > New Revision: 235991
> >> >
> >> > URL: http://llvm.org/viewvc/llvm-project?rev=235991&view=rev
> >> > Log:
> >> > Replace sprintf with snprintf to avoid a crash.
> >> > During testing -data-list-register-values, I saw a crash here due to
> >> > buffer overflow.
> >> > This commit should fix the crash. There is still problem with printing
> >> > 1-byte register in some cases that I will fix separately.
> >> >
> >> > No regression on MI test cases.
> >> >
> >> >
> >> > Modified:
> >> >     lldb/trunk/tools/lldb-mi/MIUtilString.cpp
> >> >
> >> > Modified: lldb/trunk/tools/lldb-mi/MIUtilString.cpp
> >> > URL:
> >> >
> http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-mi/MIUtilStr
> >> > ing.cpp?rev=235991&r1=235990&r2=235991&view=diff
> >> >
> >> ================================================================
> >> ======
> >> > ========
> >> > --- lldb/trunk/tools/lldb-mi/MIUtilString.cpp (original)
> >> > +++ lldb/trunk/tools/lldb-mi/MIUtilString.cpp Tue Apr 28 09:16:00 2015
> >> > @@ -17,6 +17,7 @@
> >> >
> >> >  // In-house headers:
> >> >  #include "MIUtilString.h"
> >> > +#include "Platform.h"
> >> >
> >> >  //++
> >> > ----------------------------------------------------------------------
> >> > --------------  // Details: CMIUtilString constructor.
> >> > @@ -844,8 +845,9 @@ CMIUtilString::Escape(const bool vbEscap
> >> >                      strNew.push_back(cUnescapedChar);
> >> >                  else
> >> >                  {
> >> > -                    char strEscapedChar[sizeof("\\xXX")];
> >> > -                    ::sprintf(strEscapedChar, "\\x%02" PRIx8,
> >> > cUnescapedChar);
> >> > +                    const size_t size = sizeof("\\xXX");
> >> > +                    char strEscapedChar[size];
> >> > +                    ::snprintf(strEscapedChar, size, "\\x%02" PRIx8,
> >> > cUnescapedChar);
> >> >                      strNew.append(strEscapedChar);
> >> >                  }
> >> >                  break;
> >> >
> >> >
> >> > _______________________________________________
> >> > lldb-commits mailing list
> >> > lldb-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> >> >
> >> >
> >> >
> >> >
> >> > _______________________________________________
> >> > lldb-commits mailing list
> >> > lldb-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
> >> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150428/e3ef8131/attachment.html>