Last Comment Bug 783703 - Don't use _snprintf for snprintf on Windows! (mtransport)
: Don't use _snprintf for snprintf on Windows! (mtransport)
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: WebRTC: Networking (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: ---
Assigned To: Ted Mielczarek [:ted.mielczarek]
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks: 699646
  Show dependency treegraph
 
Reported: 2012-08-17 15:38 PDT by [:jesup] on pto until 2016/7/5 Randell Jesup
Modified: 2012-12-22 08:22 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for snprintf (1.79 KB, patch)
2012-08-17 15:38 PDT, [:jesup] on pto until 2016/7/5 Randell Jesup
no flags Details | Diff | Review
don't use _snprintf on Windows in media/mtransport (3.43 KB, patch)
2012-08-27 13:51 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Review

Description [:jesup] on pto until 2016/7/5 Randell Jesup 2012-08-17 15:38:15 PDT
Created attachment 652958 [details] [diff] [review]
Patch for snprintf

_snprintf doesn't guarantee null-termination.  Evil.  Totally.

Applies to nrappkit, and also to dtlsidentity.cpp in mtransport.

Patch (not for upstreaming probably) attached.  Needs to be tried on windows.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2012-08-20 10:17:22 PDT
I mentioned this on #media, but there is a MSVC CRT API you can use: _snprintf_s. Unfortunately you have to pass _TRUNCATE as one of the parameters to make it work properly, so you can't just #define the problem away. I wrote (without testing) a possible inline snprintf implementation we could use:

int snprintf(char *buffer, size_t n, const char *format, ...)
{
  va_list argp;
  int ret;
  va_start(argp, fmt);
  ret = vsnprintf_s(buffer, n, _TRUNCATE, format, argp);
  va_end(argp);
  return ret;
}
Comment 2 Eric Rescorla (:ekr) 2012-08-23 11:02:06 PDT
Comment on attachment 652958 [details] [diff] [review]
Patch for snprintf

Review of attachment 652958 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/mtransport/third_party/nrappkit/src/port/win32/include/csi_platform.h
@@ +53,5 @@
>  #include <r_types.h>
>  
>  #define strcasecmp _stricmp
>  #define strncasecmp _strnicmp
> +#define snprintf PR_snprintf

I'd rather not pull NSPR in here....

Let's just write a new wrapped version of snprintf() and put it in util/util.c

Do you need me to do that?
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-08-27 13:51:44 PDT
Created attachment 655750 [details] [diff] [review]
don't use _snprintf on Windows in media/mtransport

I fixed up jesup's patch to use the snprintf implementation I pasted above.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2012-08-27 14:03:28 PDT
https://hg.mozilla.org/projects/alder/rev/6b00aa578af2

Note You need to log in before you can comment on or make changes to this bug.