Re: Bug#651529: rsyslog: FTBFS on hurd-i386
On Tue, 2011-12-13 at 06:41 +0100, Guillem Jover wrote:
> 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
...
> > 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
...
> > @@ -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.
Fixed!
> > } 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.
Fixed!
> > }
> > }
> >
> > /* ... 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.
This is a matter of how much you should change in the original code. I
thought about it but decided not to by then. Now the updated patch does.
> >
> > /* now see if we have an extension and, if not, append ".so" */
> > - if(!bHasExtension) {
> > + if(bHasExtension) {
...
> > /* 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.
This was fixed by an update to the patch, sent to the bug number but not
to the debian-hurd ML.
> 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.
See above!
--- 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-13 09:25:16.000000000 +0100
@@ -767,9 +767,9 @@
DEFiRet;
size_t iPathLen, iModNameLen;
- uchar szPath[PATH_MAX];
+ uchar *szPath = NULL;
uchar *pModNameCmp;
- int bHasExtension;
+ int bHasExtension, pHasSlash;
void *pModHdlr, *pModInit;
modInfo_t *pModInfo;
uchar *pModDirCurr, *pModDirNext;
@@ -805,12 +805,16 @@
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 */
+ *szPath = '\0'; /* we do not need to append the path - its already in the module name */
iPathLen = 0;
} else {
- *szPath = '\0';
-
iPathLen = strlen((char *)pModDirCurr);
+ if(pModDirCurr[iPathLen - 1] == '/')
+ pHasSlash = TRUE;
+ else {
+ iPathLen += iPathLen;
+ pHasSlash = FALSE;
+ }
pModDirNext = (uchar *)strchr((char *)pModDirCurr, ':');
if(pModDirNext)
iPathLen = (size_t)(pModDirNext - pModDirCurr);
@@ -821,45 +825,38 @@
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 != NULL)
+ free(szPath);
+ iPathLen = iPathLen + iModNameLen + 3;
+ 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);
- }
+ if(!pHasSlash) {
+ szPath[iPathLen - 2] = '/';
+ szPath[iPathLen - 1] = '\0';
}
}
/* ... add actual name ... */
- strncat((char *) szPath, (char *) pModName, sizeof(szPath) - iPathLen - 1);
-
/* 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 +900,7 @@
}
finalize_it:
+ free(szPath);
pthread_mutex_unlock(&mutLoadUnload);
RETiRet;
}
Reply to: