Closed Bug 783703 Opened 9 years ago Closed 9 years ago

Don't use _snprintf for snprintf on Windows! (mtransport)

Categories

(Core :: WebRTC: Networking, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jesup, Assigned: ted)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch Patch for snprintf (obsolete) — Splinter Review
_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.
Attachment #652958 - Flags: review?(ekr)
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;
}
Blocks: 699646
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?
I fixed up jesup's patch to use the snprintf implementation I pasted above.
Attachment #655750 - Flags: review?(ekr)
Attachment #652958 - Attachment is obsolete: true
Attachment #652958 - Flags: review?(ekr)
Assignee: ekr → ted.mielczarek
https://hg.mozilla.org/projects/alder/rev/6b00aa578af2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
Attachment #655750 - Flags: review?(ekr)
You need to log in before you can comment on or make changes to this bug.