On Tue, 21 Sep 2010 at 19:04:04 +0200, Tamas TEVESZ wrote:
>
> please eye this (will turn it into a proper git patch once it's deemed
> committable)
First, thanks a lot for doing these non-trivial changes.
But it's a big patch with several (unrelated) changes, making it somehow
difficult to read through. The overall result seems to be quite nice, AFAICT.
Nothing will beat the test of time, though :-)
But for bisectability and review purposes, it should be splitted a bit.
I realize it may be a pain to separate it now, but it's worth it.
> - introduce the use of strlcat and strlcpy, bundle a copy for systems
> not having it (XXX use libbsd on linuxes that have it)
This one deserves a patch of its own. I think it's even better if you
make it in two phases. First patch you introduce the functions with the
configure.ac detection. Then another patch to convert to the new functions
throughout the code.
> - change wmalloc() to zero the allocated memory, clean up various
> forms of zeroing throughout
Another separate patch
> - wapplication.c:checkFile() returns NULL on several cases of NULLs in
> its paramlist (this is not even an api change, unless you consider
> segfault->sensible result an api change), which made it possible to
another
> - massively simplify wapplication.c:WMPathForResourceOfType() (no more
> redundant getenv()s)
another
> - wfilepanel.c:deleteFile() lost about a thousand pounds, while
> gaining some correctness (why hand-roll strerror if we have one for free?)
This one could also have its own patch.
Comments below.
> @@ -251,9 +253,16 @@ char *wfindfile(char *paths, char *file)
> path = wmalloc(len + flen + 2);
> path = memcpy(path, tmp, len);
> path[len] = 0;
> - if (path[len - 1] != '/')
> - strcat(path, "/");
> - strcat(path, file);
> + if (path[len - 1] != '/') {
> + if (strlcat(path, "/", len + flen + 2) >= len + flen +
> 2) {
> + wfree(path);
> + return NULL;
> + }
> + }
> + if (strlcat(path, file, len + flen + 2) >= len + flen + 2) {
> + wfree(path);
> + return NULL;
> + }
> fullpath = wexpandpath(path);
> wfree(path);
> if (fullpath) {
> @@ -301,8 +310,11 @@ char *wfindfileinlist(char **path_list, char *file)
> path = wmalloc(len + flen + 2);
> path = memcpy(path, path_list[i], len);
> path[len] = 0;
> - strcat(path, "/");
> - strcat(path, file);
> + if (strlcat(path, "/", len + flen + 2) >= len + flen + 2 ||
> + strlcat(path, file, len + flen + 2) >= len + flen + 2) {
> + wfree(path);
> + return NULL;
> + }
> /* expand tilde */
> fullpath = wexpandpath(path);
> wfree(path);
> @@ -358,8 +370,11 @@ char *wfindfileinarray(WMPropList * array, char *file)
> path = wmalloc(len + flen + 2);
> path = memcpy(path, p, len);
> path[len] = 0;
> - strcat(path, "/");
> - strcat(path, file);
> + if (strlcat(path, "/", len + flen + 2) >= len + flen + 2 ||
> + strlcat(path, file, len + flen + 2) >= len + flen + 2) {
> + wfree(path);
> + return NULL;
> + }
It's cleaner to use the 'goto' here.
> @@ -709,15 +700,20 @@ char *WMGetBrowserPathToColumn(WMBrowser * bPtr, int
> column)
> }
>
> /* get the path */
> - path = wmalloc(size + (column + 1) * strlen(bPtr->pathSeparator) + 1);
> - /* ignore first / */
> - *path = 0;
> + slen = size + (column + 1) * strlen(bPtr->pathSeparator) + 1;
> + path = wmalloc(slen);
> for (i = 0; i <= column; i++) {
> - strcat(path, bPtr->pathSeparator);
> + if (strlcat(path, bPtr->pathSeparator, slen) >= slen) {
> + wfree(path);
> + return NULL;
> + }
> item = WMGetListSelectedItem(bPtr->columns[i]);
> if (!item)
> break;
> - strcat(path, item->text);
> + if (strlcat(path, item->text, slen) >= slen) {
> + wfree(path);
> + return NULL;
> + }
> }
and perhaps here too
> static char *createTruncatedString(WMFont * font, char *text, int *textLen,
> int width)
> {
> - int dLen = WMWidthOfString(font, ".", 1);
> - char *textBuf = (char *)wmalloc((*textLen) + 4);
> + size_t slen;
> + int dLen;
> + char *textBuf;
> +
> + dLen = WMWidthOfString(font, ".", 1);
> + slen = *textLen + 4;
> + textBuf = wmalloc(slen);
>
> if (width >= 3 * dLen) {
> - int dddLen = 3 * dLen;
> int tmpTextLen = *textLen;
>
> - strcpy(textBuf, text);
> - while (tmpTextLen && (WMWidthOfString(font, textBuf,
> tmpTextLen) + dddLen > width))
> + if (strlcpy(textBuf, text, slen) >= slen) {
> + wfree(textBuf);
> + return NULL;
> + }
> + while (tmpTextLen && (WMWidthOfString(font, textBuf,
> tmpTextLen) + 3 * dLen > width))
> tmpTextLen--;
> - strcpy(textBuf + tmpTextLen, "...");
> + if (strlcpy(textBuf + tmpTextLen, "...", slen) >= slen) {
> + wfree(textBuf);
> + return NULL;
> + }
> *textLen = tmpTextLen + 3;
> } else if (width >= 2 * dLen) {
> - strcpy(textBuf, "..");
> + if (strlcpy(textBuf, "..", slen) >= slen) {
> + wfree(textBuf);
> + return NULL;
> + }
> *textLen = 2;
> } else if (width >= dLen) {
> - strcpy(textBuf, ".");
> + if (strlcpy(textBuf, ".", slen) >= slen) {
> + wfree(textBuf);
> + return NULL;
> + }
and definitely here.
--
To unsubscribe, send mail to [email protected].
