Re: Bug#651529: rsyslog: FTBFS on hurd-i386
Hi!
On Fri, 2011-12-09 at 16:40:41 +0100, Svante Signell wrote:
> Source: rsyslog
> Version: 5.8.6-1
> Severity: important
> Tags: patch
> User: debian-hurd@lists.debian.org
> Usertags: hurd
> rsyslog FTBFS on GNU/Hurd due to the usage of PATH_MAX in one function
> static rsRetVal Load(uchar *pModName) in file realtime/modules.c. The
> attached patch use dynamic allocation of strings instead of fixed length
> ones. The number of changes are maybe on the order to check with
> upstream if they are OK. A test program for the construct has been
> created and works. The built package has also been installed kvm box and
> been running there for a while and does also work.
> diff -ur rsyslog-5.8.6/runtime/modules.c rsyslog-5.8.6.buggy/runtime/modules.c
> --- rsyslog-5.8.6/runtime/modules.c 2011-10-21 11:53:02.000000000 +0200
> +++ rsyslog-5.8.6.modified/runtime/modules.c 2011-12-09 14:40:08.000000000 +0100
> @@ -767,7 +767,7 @@
> DEFiRet;
>
> size_t iPathLen, iModNameLen;
> - uchar szPath[PATH_MAX];
> + uchar *szPath = NULL;
> uchar *pModNameCmp;
> int bHasExtension;
> void *pModHdlr, *pModInit;
> @@ -805,11 +805,8 @@
> do {
> /* now build our load module name */
> if(*pModName == '/' || *pModName == '.') {
> - *szPath = '\0'; /* we do not need to append the path - its already in the module name */
> iPathLen = 0;
On this case no memory is allocated which will cause a segfault,
further on when writing to szPath.
> } else {
> - *szPath = '\0';
> -
> iPathLen = strlen((char *)pModDirCurr);
> pModDirNext = (uchar *)strchr((char *)pModDirCurr, ':');
> if(pModDirNext)
> @@ -821,45 +818,41 @@
> continue;
> }
> break;
> - } else if(iPathLen > sizeof(szPath) - 1) {
> - errmsg.LogError(0, NO_ERRCODE, "could not load module '%s', module path too long\n", pModName);
> + }
> + if((szPath = malloc(iPathLen+1)) == NULL) {
> + errmsg.LogError(0, NO_ERRCODE, "could not allocate memory '%s'\n", pModName);
> ABORT_FINALIZE(RS_RET_MODULE_LOAD_ERR_PATHLEN);
> }
>
> - strncat((char *) szPath, (char *)pModDirCurr, iPathLen);
> - iPathLen = strlen((char*) szPath);
> + strcpy((char *) szPath, (char *)pModDirCurr);
>
> if(pModDirNext)
> pModDirCurr = pModDirNext + 1;
>
> if((szPath[iPathLen - 1] != '/')) {
> - if((iPathLen <= sizeof(szPath) - 2)) {
> - szPath[iPathLen++] = '/';
> - szPath[iPathLen] = '\0';
> - } else {
> - errmsg.LogError(0, RS_RET_MODULE_LOAD_ERR_PATHLEN, "could not load module '%s', path too long\n", pModName);
> - ABORT_FINALIZE(RS_RET_MODULE_LOAD_ERR_PATHLEN);
> - }
> + szPath[iPathLen++] = '/';
> + szPath[iPathLen] = '\0';
This will write past the last character. You need to allocate +1
additional byte for it.
> }
> }
>
> /* ... add actual name ... */
> - strncat((char *) szPath, (char *) pModName, sizeof(szPath) - iPathLen - 1);
> + iPathLen = iPathLen + iModNameLen + 3;
> + if((szPath = realloc(szPath, iPathLen+1)) == NULL) {
> + errmsg.LogError(0, NO_ERRCODE, "could not allocate memory '%s'\n", pModName);
> + ABORT_FINALIZE(RS_RET_MODULE_LOAD_ERR_PATHLEN);
> + }
This will do useless work that could be avoided. Pre-compute the total
path len and use a single malloc() instead.
>
> /* now see if we have an extension and, if not, append ".so" */
> - if(!bHasExtension) {
> + if(bHasExtension) {
> + strncat((char *) szPath, (char *) pModName, iModNameLen+3);
> + } else {
> /* we do not have an extension and so need to add ".so"
> * TODO: I guess this is highly importable, so we should change the
> * algo over time... -- rgerhards, 2008-03-05
> */
> /* ... so now add the extension */
> - strncat((char *) szPath, ".so", sizeof(szPath) - strlen((char*) szPath) - 1);
> - iPathLen += 3;
> - }
> -
> - if(iPathLen + strlen((char*) pModName) >= sizeof(szPath)) {
> - errmsg.LogError(0, RS_RET_MODULE_LOAD_ERR_PATHLEN, "could not load module '%s', path too long\n", pModName);
> - ABORT_FINALIZE(RS_RET_MODULE_LOAD_ERR_PATHLEN);
> + strncat((char *) szPath, (char *) pModName, iModNameLen);
> + strncat((char *) szPath, ".so", 3);
> }
>
> /* complete load path constructed, so ... GO! */
> @@ -903,6 +896,7 @@
> }
>
> finalize_it:
> + free(szPath);
> pthread_mutex_unlock(&mutLoadUnload);
> RETiRet;
> }
It seems (w/o looking at the full function) this will leak if the loop is
repeated for whatever reason, and szPath will get malloc()ed over.
The function can probably be simplified quite a bit, by allocating
once at the beginning of the loop and then using sprintf() to build
the path.
thanks,
guillem
Reply to: