The title of commit 2 isn't very descriptive ("restructure" how?), and in fact is misleading, since it contains functional changes as well. The "save_str" local variable seems redundant; it's never modified. Patches 3 and 4 have "shlwapi" as a prefix; that should probably be "kernelbase" instead.
From patch 3:
+ /* + * url has to contain at least one colon. + * The scheme part length has a minimum of 2 characters. + * Output buffer length has to be minimum of 3 characters, scheme + colon. + * + * Return S_FALSE if url and buffer do not meet this minimum requirements. + */ + helper_str = wcschr(url, L':'); + if (!helper_str) + return S_FALSE;
"helper_str" is never used in this patch. The comment makes reference to scheme length but doesn't validate it. The line about S_FALSE strikes me as somewhat unnecessary; the code tells us as much.
From patch 4:
+ /* + * Colon should not be at the beginning + */
No need for these comments to span three lines.
+ colon_pos = helper_str-url; + if (helper_str && (1 >= colon_pos)) + return S_FALSE;
I find conditionals hard to read when phrased like this; can we please put the constant on the right side?
+ /* + * Check if string fits into maxChars + */
This comment, and others like it, seems redundant; the code tells us as much.
+ // TODO: Use own buffer for adjustments, output buffer may not fit before fix
We don't use C++ comments.
+ if (colon_pos >= maxChars && maxChars < 3) + return S_FALSE;
Why the "maxChars < 3" part?
+ /* + * Set potential scheme + */ + lstrcpynW(save_str, url, pos+2); + url = url + colon_pos + 1;
This looks wrong. Either the following loop will overwrite this, or we'll return S_FALSE, in which case we probably shouldn't be touching the output buffer. In fact, should we even be writing the scheme if we might fail later?
+ /* Return false in most remaining cases should be safe */ + return S_FALSE;
This comment seems out of place. Either it's correct or it's not.
+ /* + * Concat L"://"" for ftp, http, https scheme * else ":" + */
This all seems irrelevant to the patch subject; it should be part of its own patch. This commit also doesn't quite match the code. We already copied the colon; we're not doing it here.
+ if ( 0 == lstrcmpW(save_str, L"ftp:") || + 0 == lstrcmpW(save_str, L"http:") || + 0 == lstrcmpW(save_str, L"https:") )
Let's please consistently use standard wcs* functions, instead of mixing lstr* and wcs*. I would also just simply say "!wcscmp", but I guess to each their own.
+ { + if ( url[0] == L'\\' || url[0] == L'/' ) + { + url++; + if ( url[0] == L'\\' || url[0] == L'/' ) + url++; + } + lstrcatW(save_str, L"//");
This is missing destination size checks. Also, if we don't have enough room, should we really be touching the output buffer earlier, viz. to write the scheme?
+ } + /* + * Remove leading "/", "\" and ":" from url. + * Output already have them if needed + */ + while ( url[0] == L'\\' || url[0] == L'/' ) + { + lstrcatW(save_str, L"/"); + url++; + }
The comment doesn't match the code here, and there's no tests for this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1825#note_22212