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

[Pavel Labath]

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
>> >