You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Dirk-Willem van Gulik <di...@webweaving.org> on 2016/11/16 18:23:41 UTC
debug patch (was FOSSA)
That is a really rather odd bit of code. First strawman to improve things a bit below.
Dw.
Index: misc/win32/misc.c
===================================================================
--- misc/win32/misc.c (revision 1765030)
+++ misc/win32/misc.c (working copy)
@@ -181,16 +181,34 @@
if (tlsid == 0xFFFFFFFF) {
tlsid = (TlsAlloc)();
}
+ if (tlsid == TLS_OUT_OF_INDEXES) {
+ char *err = "apr_dbg_log() internal error: TLS_OUT_OF_INDEXES";
+ (EnterCriticalSection)(&cs);
+ (WriteFile)(fh, err, (DWORD)strlen(err), &wrote, NULL);
+ (LeaveCriticalSection)(&cs);
+ return ha;
+ }
sbuf = (TlsGetValue)(tlsid);
if (!fh || !sbuf) {
sbuf = (malloc)(1024);
+ if (!sbuf) {
+ char *err = "apr_dbg_log() internal error: malloc failed.";
+ (EnterCriticalSection)(&cs);
+ (WriteFile)(fh, err, (DWORD)strlen(err), &wrote, NULL);
+ (LeaveCriticalSection)(&cs);
+ return ha;
+ }
(TlsSetValue)(tlsid, sbuf);
- sbuf[1023] = '\0';
if (!fh) {
- (GetModuleFileNameA)(NULL, sbuf, 250);
- sprintf(strchr(sbuf, '\0'), ".%u",
- (unsigned int)(GetCurrentProcessId)());
+ char fnamebuff[251];
+ (GetModuleFileNameA)(NULL, fnamebuff, sizeof(fnamebuff)-1);
+ // The string is truncated to nSize characters and is not
+ // null-terminated (on WinXP, fine on modern windows versions).
+ fnamebuff[sizeof(fnamebuff)-1] = '\0';
+
+ snprintf(sbuf, sizeof(sbuf), "%s.%u",
+ fnamebuff, (signed int)(GetCurrentProcessId)());
fh = (CreateFileA)(sbuf, GENERIC_WRITE, 0, NULL,
CREATE_ALWAYS, 0, NULL);
(InitializeCriticalSection)(&cs);
@@ -198,7 +216,7 @@
}
if (!nh) {
- (sprintf)(sbuf, "%p %08x %08x %s() %s:%d\n",
+ (snprintf)(sbuf, sizeof)(sbuf), "%p %08x %08x %s() %s:%d\n",
ha, (unsigned int)seq, (unsigned int)GetCurrentThreadId(),
fn, fl, ln);
(EnterCriticalSection)(&cs);
@@ -226,7 +244,7 @@
dsc = "Timed Out";
}
}
- (sprintf)(sbuf, "%p %08x %08x %s(%s) %s:%d\n",
+ (snprintf)(sbuf, sizeof(sbuf), "%p %08x %08x %s(%s) %s:%d\n",
*hv, (unsigned int)seq,
(unsigned int)GetCurrentThreadId(),
fn, dsc, fl, ln);
Re: debug patch (was FOSSA)
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
Better to limit the individual %s args to ensure the entire string fits in
the allocated 1024 char buffer than switch to snprintf, IMO. These are fn
name constants, etc that can't conceivably overflow in the first place.
On Nov 16, 2016 19:24, "Dirk-Willem van Gulik" <di...@webweaving.org> wrote:
> That is a really rather odd bit of code. First strawman to improve things
> a bit below.
>
> Dw.
>
> Index: misc/win32/misc.c
> ===================================================================
> --- misc/win32/misc.c (revision 1765030)
> +++ misc/win32/misc.c (working copy)
> @@ -181,16 +181,34 @@
> if (tlsid == 0xFFFFFFFF) {
> tlsid = (TlsAlloc)();
> }
> + if (tlsid == TLS_OUT_OF_INDEXES) {
> + char *err = "apr_dbg_log() internal error: TLS_OUT_OF_INDEXES";
> + (EnterCriticalSection)(&cs);
> + (WriteFile)(fh, err, (DWORD)strlen(err), &wrote, NULL);
> + (LeaveCriticalSection)(&cs);
> + return ha;
> + }
>
> sbuf = (TlsGetValue)(tlsid);
> if (!fh || !sbuf) {
> sbuf = (malloc)(1024);
> + if (!sbuf) {
> + char *err = "apr_dbg_log() internal error: malloc failed.";
> + (EnterCriticalSection)(&cs);
> + (WriteFile)(fh, err, (DWORD)strlen(err), &wrote, NULL);
> + (LeaveCriticalSection)(&cs);
> + return ha;
> + }
> (TlsSetValue)(tlsid, sbuf);
> - sbuf[1023] = '\0';
> if (!fh) {
> - (GetModuleFileNameA)(NULL, sbuf, 250);
> - sprintf(strchr(sbuf, '\0'), ".%u",
> - (unsigned int)(GetCurrentProcessId)());
> + char fnamebuff[251];
> + (GetModuleFileNameA)(NULL, fnamebuff, sizeof(fnamebuff)-1);
> + // The string is truncated to nSize characters and is not
> + // null-terminated (on WinXP, fine on modern windows
> versions).
> + fnamebuff[sizeof(fnamebuff)-1] = '\0';
> +
> + snprintf(sbuf, sizeof(sbuf), "%s.%u",
> + fnamebuff, (signed int)(GetCurrentProcessId)());
> fh = (CreateFileA)(sbuf, GENERIC_WRITE, 0, NULL,
> CREATE_ALWAYS, 0, NULL);
> (InitializeCriticalSection)(&cs);
> @@ -198,7 +216,7 @@
> }
>
> if (!nh) {
> - (sprintf)(sbuf, "%p %08x %08x %s() %s:%d\n",
> + (snprintf)(sbuf, sizeof)(sbuf), "%p %08x %08x %s() %s:%d\n",
> ha, (unsigned int)seq, (unsigned
> int)GetCurrentThreadId(),
> fn, fl, ln);
> (EnterCriticalSection)(&cs);
> @@ -226,7 +244,7 @@
> dsc = "Timed Out";
> }
> }
> - (sprintf)(sbuf, "%p %08x %08x %s(%s) %s:%d\n",
> + (snprintf)(sbuf, sizeof(sbuf), "%p %08x %08x %s(%s) %s:%d\n",
> *hv, (unsigned int)seq,
> (unsigned int)GetCurrentThreadId(),
> fn, dsc, fl, ln);
>
>