[stlport-bugs] [ stlport-Bugs-1674974 ] invalid use of strncpy_s in __Named_exception constructor

[SourceForge.net <no...@so...>]

Bugs item #1674974, was opened at 2007-03-06 16:15
Message generated for change (Comment added) made by dums
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=766244&aid=1674974&group_id=146814

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Operational Environment/Compiler specific code
Group: None
Status: Closed
Resolution: None
Priority: 5
Private: No
Submitted By: Bronek Kozicki (brok_k)
Assigned to: Francois Dumont (dums)
Summary: invalid use of strncpy_s in __Named_exception constructor

Initial Comment:
This problem is specific to Visual C++ 8 (ie. Visual Studio 2005) when using STLPort 5.1.x (including 5.1.2) linked as a dynamic library. The easiest way to demonstrate it is to build and run following program:

#include <stdexcept>
#include <string>

int main()
{
  try
  {
    // try to throw a very long exception
    char m[500] = {0};
    for (int i = 0; i < sizeof(m) - 1; ++i)
      m[i] = 'a';
    throw std::runtime_error(m);
  }
  catch(std::exception &)
  {
    // we never reach this point
    return 1;
  }
  return 0;
}

Result of this program in Debug build is failed assertion "Buffer is too small" in C runtime followed by uncaught exception followed by termination; in Release build it is uncaught exception followed by process termination. The cause of the error is line 118 in dll_main.cpp , where actual size of string (exception description) is passed to strncpy_s as the last parameter. According to MSDN http://msdn2.microsoft.com/en-us/library/5dae5d43(VS.80).aspx , if this parameter is larger than buffer size (256 in this case) less 1 (space for nul termination) the invalid parameter handler is invoked, which by default will crash the process. Proposed fix is to replace last parameter with _S_bufsize - 1 , or use _TRUNCATE (probably less portable solution). BTW, third parameter (two first parameters are disguised by _STLP_ARRAY_AND_SIZE macro) should use c_str() rather than data(), but it's irrelevant here.


----------------------------------------------------------------------

>Comment By: Francois Dumont (dums)
Date: 2007-03-10 12:08

Message:
Logged In: YES 
user_id=1096600
Originator: NO

I still don't see the reason for such a check. What is the problem of
using safe string functions internally ? Is it for performance
consideration ?

----------------------------------------------------------------------

Comment By: Petr Ovtchenkov (complement)
Date: 2007-03-09 17:29

Message:
Logged In: YES 
user_id=615813
Originator: NO

In trunk. Pls, verify.

----------------------------------------------------------------------

Comment By: Bronek Kozicki (brok_k)
Date: 2007-03-09 17:06

Message:
Logged In: YES 
user_id=843653
Originator: YES

Petr, this is exactly why I picked name _CRT_SECURE_NO_DEPRECATE - it's
already defined by those who do not wish to use safe string functions, as
documented in MSDN. You do not need to include any new define of your own
in the project.

----------------------------------------------------------------------

Comment By: Petr Ovtchenkov (complement)
Date: 2007-03-09 15:40

Message:
Logged In: YES 
user_id=615813
Originator: NO

>  but to give users an option not to use safe string
functions if they do not wish to

Well, massive usage of such fine-tuning settings underwhelm me. We already
have a lot of defines for various features... 

----------------------------------------------------------------------

Comment By: Bronek Kozicki (brok_k)
Date: 2007-03-09 14:40

Message:
Logged In: YES 
user_id=843653
Originator: YES

Francis, the purpose of this proposed check is not to prevent any
potential problems, but to give users an option not to use safe string
functions if they do not wish to. 

----------------------------------------------------------------------

Comment By: Francois Dumont (dums)
Date: 2007-03-09 10:46

Message:
Logged In: YES 
user_id=1096600
Originator: NO

The fact that we are using those functions internally in STLport do not
mean that STLport users have to use it. Unless you tell me that MSVC users
can disable safe string functions that might break STLport build/use, I do
not see why we would need this kind of check.

----------------------------------------------------------------------

Comment By: Bronek Kozicki (brok_k)
Date: 2007-03-08 14:46

Message:
Logged In: YES 
user_id=843653
Originator: YES

Francois, thank you. I wonder if you could add check in config/_msvc.h if
_CRT_SECURE_NO_DEPRECATE is set and revert to old string functions if it
is, like: 

#if (_STLP_MSVC_LIB >= 1400) && !defined
(_STLP_USING_PLATFORM_SDK_COMPILER) && !defined(UNDER_CE) &&
!defined(_CRT_SECURE_NO_DEPRECATE)
#  define _STLP_USE_SAFE_STRING_FUNCTIONS 1
#endif

This way those who do not want these functions will not use them. Thanks
for your good work!



----------------------------------------------------------------------

Comment By: Francois Dumont (dums)
Date: 2007-03-08 09:22

Message:
Logged In: YES 
user_id=1096600
Originator: NO

I just remember what has been the great interest in using string safe
functions. It is not really the check performed by the functions themselves
but rather the fact that to use those you have to carry the buffer size
along with the buffer pointer. AFAIR it revealed some potential buffer
overrun in STLport.

----------------------------------------------------------------------

Comment By: Francois Dumont (dums)
Date: 2007-03-07 21:37

Message:
Logged In: YES 
user_id=1096600
Originator: NO

Sorry but I hadn't follow your discussion guy.

I agree with first Petr remark, use of data() was ok in current code, it
is only invalid when using _TRUNCATE

I finally use _TRUNCATE, for the moment only MSVC comes with secure string
functions so I do not expect any portability issue. We will see when other
runtime will come with it.

Motivation for using secure string functions was that MSVC8 was reporting
all use of strncpy or the like as deprecated functions. Rather than disable
warnings I always prefer to find a way to fix those.

Fix is in STLPORT_5_1 SVN branch for the moment.


----------------------------------------------------------------------

Comment By: Bronek Kozicki (brok_k)
Date: 2007-03-07 08:41

Message:
Logged In: YES 
user_id=843653
Originator: YES

Here is another $0.02 - behaviour described in MSDN is not 100% portable
(in the sense that _TRUNCATE is nonstandard). See also page 50 in draft
document published by C standard committee
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1135.pdf 

----------------------------------------------------------------------

Comment By: Petr Ovtchenkov (complement)
Date: 2007-03-07 03:01

Message:
Logged In: YES 
user_id=615813
Originator: NO

<snip>
errno_t strncpy_s(
   char *strDest,
   size_t numberOfElements,
   const char *strSource,
   size_t count
);
</snip>

<snip>
These functions try to copy the first D characters of strSource to
strDest, where D is the lesser of count and the length of strSource. If
those D characters will fit within strDest (whose size is given as
numberOfElements) and still leave room for a null terminator, then those
characters are copied and a terminating null is appended; otherwise,
strDest[0] is set to the null character and the invalid parameter handler
is invoked, as described in Parameter Validation.

There is an exception to the above paragraph. If count is _TRUNCATE, then
as much of strSource as will fit into strDest is copied while still leaving
room for the terminating null which is always appended.

For example,

char dst[5];

strncpy_s(dst, 5, "a long string", 5);

means that we are asking strncpy_s to copy five characters into a buffer
five bytes long; this would leave no space for the null terminator, hence
strncpy_s zeroes out the string and calls the invalid parameter handler.

If truncation behavior is needed, use _TRUNCATE or (size – 1):

strncpy_s(dst, 5, "a long string", _TRUNCATE);

strncpy_s(dst, 5, "a long string", 4);

Note that unlike strncpy, if count is greater than the length of
strSource, the destination string is NOT padded with null characters up to
length count.

The behavior of strncpy_s is undefined if the source and destination
strings overlap.
</snip>

Hmm, looks like MS's supersafefunctions not just duplicate original string
functions, supersafefunctions are really implants. Looks you right. Thanks
for $0.02 in my penny bank of args against MS.

----------------------------------------------------------------------

Comment By: Bronek Kozicki (brok_k)
Date: 2007-03-06 21:03

Message:
Logged In: YES 
user_id=843653
Originator: YES

using _S_bufsize - 1 as a last parameter is essentially a lie. It is safe
as long as string is null terminated, but data() does not provide such
warranty (as opposed to c_str() ). As to your question - the point of
"safe" functions is to provide additional validation of parameters. If this
validation fails, "interesting things" will happen. While "interesting
things" might have also happend without such validation, the difference is
if they are exploitable or not. If you ask me, I think that not all uses of
these functions in STLPort (including the one discussed here) are
justified.


----------------------------------------------------------------------

Comment By: Petr Ovtchenkov (complement)
Date: 2007-03-06 17:53

Message:
Logged In: YES 
user_id=615813
Originator: NO

> If last parameter is updated (???) to _S_bufsize - 1 then using data()
instead of c_str() is potentialy (???) dangerous (????).

I don't understand you. And then, what the meaning of 'supersafe string
functions'?




----------------------------------------------------------------------

Comment By: Bronek Kozicki (brok_k)
Date: 2007-03-06 17:43

Message:
Logged In: YES 
user_id=843653
Originator: YES

If last parameter is updated to _S_bufsize - 1 then using data() instead
of c_str() is potentialy dangerous.

----------------------------------------------------------------------

Comment By: Petr Ovtchenkov (complement)
Date: 2007-03-06 16:46

Message:
Logged In: YES 
user_id=615813
Originator: NO

> should use c_str() rather than data()

data() is ok: due to following size().

BTW, Francois, is "SAFE_STRING_FUNCTIONS" are so useful? IMO MS's 'better
security' is bogus idea (hehe, more secure that strncpy, we double check
you size, or event quadruple...)


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=766244&aid=1674974&group_id=146814