urlmon: Fix buffer overflow in parse_canonicalize.

dlls/urlmon/tests/uri.c

@@ -7405,6 +7405,11 @@ static inline LPWSTR a2w(LPCSTR str) {
     return ret;
 }
 
+static inline void *heap_alloc(size_t len)
+{
+    return HeapAlloc(GetProcessHeap(), 0, len);
+}
+
 static inline BOOL heap_free(void* mem) {
     return HeapFree(GetProcessHeap(), 0, mem);
 }
@@ -10403,7 +10408,9 @@ static void test_CoInternetParseIUri_InvalidArgs(void) {
     HRESULT hr;
     IUri *uri = NULL;
     WCHAR tmp[3];
+    WCHAR *longurl, *copy;
     DWORD result = -1;
+    DWORD i, len;
 
     hr = pCoInternetParseIUri(NULL, PARSE_CANONICALIZE, 0, tmp, 3, &result, 0);
     ok(hr == E_INVALIDARG, "Error: CoInternetParseIUri returned 0x%08x, expected 0x%08x.\n",
@@ -10459,6 +10466,33 @@ static void test_CoInternetParseIUri_InvalidArgs(void) {
             expected_len, result);
     }
     if(uri) IUri_Release(uri);
+
+    /* a long url that causes a crash on Wine */
+    len = INTERNET_MAX_URL_LENGTH*2;
+    longurl = heap_alloc((len+1)*sizeof(WCHAR));
+    memcpy(longurl, http_urlW, sizeof(http_urlW));
+    for(i = sizeof(http_urlW)/sizeof(WCHAR)-1; i < len; i++)
+        longurl[i] = 'x';
+    longurl[len] = 0;
+
+    copy = heap_alloc((len+1)*sizeof(WCHAR));
+    memcpy(copy, longurl, (len+1)*sizeof(WCHAR));
+
+    hr = pCreateUri(longurl, 0, 0, &uri);
+    ok(SUCCEEDED(hr), "Error: CreateUri returned 0x%08x.\n", hr);
+    if(SUCCEEDED(hr)) {
+        result = -1;
+        memset(longurl, 0xcc, len*sizeof(WCHAR));
+        hr = pCoInternetParseIUri(uri, PARSE_CANONICALIZE, 0, longurl, len+1, &result, 0);
+        ok(SUCCEEDED(hr), "Error: CoInternetParseIUri returned 0x%08x.\n", hr);
+        ok(!lstrcmpW(longurl, copy), "Error: expected long url '%s' but was '%s'.\n",
+            wine_dbgstr_w(copy), wine_dbgstr_w(longurl));
+        ok(result == len, "Error: Expected 'result' to be %d, but was %d instead.\n",
+            len, result);
+    }
+    heap_free(longurl);
+    heap_free(copy);
+    if(uri) IUri_Release(uri);
 }
 
 static void test_CoInternetParseIUri(void) {

dlls/urlmon/uri.c

@@ -6825,7 +6825,6 @@ static HRESULT parse_canonicalize(const Uri *uri, DWORD flags, LPWSTR output,
     const WCHAR *ptr = NULL;
     WCHAR *path = NULL;
     const WCHAR **pptr;
-    WCHAR buffer[INTERNET_MAX_URL_LENGTH+1];
     DWORD len = 0;
     BOOL reduce_path;
 
@@ -6852,11 +6851,11 @@ static HRESULT parse_canonicalize(const Uri *uri, DWORD flags, LPWSTR output,
          * it later.
          */
         if(reduce_path && !path && ptr == uri->canon_uri+uri->path_start)
-            path = buffer+len;
+            path = output+len;
 
         /* Check if it's time to reduce the path. */
         if(reduce_path && ptr == uri->canon_uri+uri->path_start+uri->path_len) {
-            DWORD current_path_len = (buffer+len) - path;
+            DWORD current_path_len = (output+len) - path;
             DWORD new_path_len = remove_dot_segments(path, current_path_len);
 
             /* Update the current length. */
@@ -6868,7 +6867,9 @@ static HRESULT parse_canonicalize(const Uri *uri, DWORD flags, LPWSTR output,
             const WCHAR decoded = decode_pct_val(ptr);
             if(decoded) {
                 if(allow_unescape && (flags & URL_UNESCAPE)) {
-                    buffer[len++] = decoded;
+                    if(len < output_len)
+                        output[len] = decoded;
+                    len++;
                     ptr += 2;
                     do_default_action = FALSE;
                 }
@@ -6876,48 +6877,55 @@ static HRESULT parse_canonicalize(const Uri *uri, DWORD flags, LPWSTR output,
 
             /* See if %'s needed to encoded. */
             if(do_default_action && (flags & URL_ESCAPE_PERCENT)) {
-                pct_encode_val(*ptr, buffer+len);
+                if(len + 3 < output_len)
+                    pct_encode_val(*ptr, output+len);
                 len += 3;
                 do_default_action = FALSE;
             }
         } else if(*ptr == ' ') {
             if((flags & URL_ESCAPE_SPACES_ONLY) &&
                !(flags & URL_ESCAPE_UNSAFE)) {
-                pct_encode_val(*ptr, buffer+len);
+                if(len + 3 < output_len)
+                    pct_encode_val(*ptr, output+len);
                 len += 3;
                 do_default_action = FALSE;
             }
         } else if(!is_reserved(*ptr) && !is_unreserved(*ptr)) {
             if(flags & URL_ESCAPE_UNSAFE) {
-                pct_encode_val(*ptr, buffer+len);
+                if(len + 3 < output_len)
+                    pct_encode_val(*ptr, output+len);
                 len += 3;
                 do_default_action = FALSE;
             }
         }
 
-        if(do_default_action)
-            buffer[len++] = *ptr;
+        if(do_default_action) {
+            if(len < output_len)
+                output[len] = *ptr;
+            len++;
+        }
     }
 
     /* Sometimes the path is the very last component of the IUri, so
      * see if the dot segments need to be reduced now.
      */
     if(reduce_path && path) {
-        DWORD current_path_len = (buffer+len) - path;
+        DWORD current_path_len = (output+len) - path;
         DWORD new_path_len = remove_dot_segments(path, current_path_len);
 
         /* Update the current length. */
         len -= (current_path_len-new_path_len);
     }
 
-    buffer[len++] = 0;
+    if(len < output_len)
+        output[len] = 0;
+    else
+        output[output_len-1] = 0;
 
     /* The null terminator isn't included in the length. */
-    *result_len = len-1;
-    if(len > output_len)
+    *result_len = len;
+    if(len >= output_len)
         return STRSAFE_E_INSUFFICIENT_BUFFER;
-    else
-        memcpy(output, buffer, len*sizeof(WCHAR));
 
     return S_OK;
 }