apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From William A Rowe Jr <wr...@rowe-clan.net>
Subject Re: debug patch (was FOSSA)
Date Wed, 16 Nov 2016 19:02:57 GMT
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" <dirkx@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);
>
>

Mime
View raw message