> hi,
> i discoverd a bug in vpopmail 5.3.29.
>
> in vpopmail.c in line 2737
>
> /* unlink the temp file in case tcprules has an error */
> if ( unlink(releay_template) == -1) {
> return(-1);
> }
>
> if roaming-users are enabled we get a "Error. update_rules() failed"
> everytime a user with a new ip connects. if he connects again (his ip is
> allready opened for relaying) everything runs fine. i think this is
because
> we should check " if ( unlink(releay_template) == 0) " as we have to tell
> an error IF the file releay_template-file is still there as this means
that
> tcprules has an error. if we check if unlink returns -1 we will get an
error
> if the file is NOT there but if it's not there everythink is just fine.
> michael bowe just changed a formating error for tcp.smtp.tmp.pid in the
> 5.2.29 version so i think this is why the error was not there in earlyer
> versions.
> i changed this line in my version and everything runs fine ;)
Thanks for your suggestion. You are on the right track.
When a new IP address is added to the list of permitted relays,
the tcprules program is run to rebuild the cdb file.
One of the parameters to the tcprules file is the name of a
tempfile. If the tcprules program runs successfully (ie all the
input is valid), then the tempfile is automatically removed
If some of the input is invalid, then tcprules will exit and that
tempfile will remain on the disk.
In our update_rules function, the intent of the code is to
zap the tempfile should it remain after the tcprules program
has completed running
At the moment if it doesnt find the tmpfile, then it aborts with
a "failed to update_rules" error
This is wrong. We just want to zap the file if it exists.
If it doesnt exist (which it really shouldnt in most cases),
then we should not care that the unlink command returns
an error.
Patch to fix the problem is attached.
(I also took the opportunity to do some other cosmetic
tidy ups of the roaming user code)
Michael.
--- ../vpopmail-5.3.29-orig/vpopmail.c Tue Oct 14 08:37:24 2003
+++ vpopmail.c Sun Nov 2 14:37:13 2003
@@ -45,8 +45,8 @@
#ifdef POP_AUTH_OPEN_RELAY
/* keep a output pipe to tcp.smtp file */
-int fdm;
-static char relay_template[MAX_BUFF];
+int tcprules_fdm;
+static char relay_tempfile[MAX_BUFF];
#endif
int verrori = 0;
@@ -2580,8 +2580,8 @@
char bin2[MAX_BUFF];
char *binqqargs[4];
- /* create a filename extension use when creating a tmp file */
- snprintf(relay_template, sizeof(relay_template), "tmp.%ld", (long
unsigned)getpid());
+ /* create a filename for use as a tmp file */
+ snprintf(relay_tempfile, sizeof(relay_tempfile), "%s.tmp.%ld", TCP_FILE, (long
unsigned)getpid());
/* create a pair of filedescriptors for our pipe */
if (pipe(pim) == -1) { return(-1);}
@@ -2602,7 +2602,7 @@
*/
snprintf( bin0, sizeof(bin0), "%s", TCPRULES_PROG);
snprintf( bin1, sizeof(bin1), "%s.cdb", TCP_FILE);
- snprintf( bin2, sizeof(bin2), "%s.%s", TCP_FILE, relay_template);
+ snprintf( bin2, sizeof(bin2), "%s", relay_tempfile);
/* put these strings into an argv style array */
binqqargs[0] = bin0;
@@ -2610,11 +2610,13 @@
binqqargs[2] = bin2;
binqqargs[3] = 0;
- /* run launch the command to build the new tcp rules file */
+ /* run this command now (it will sit waiting for input to be piped in */
execv(*binqqargs,binqqargs);
}
- fdm = pim[1]; close(pim[0]);
+ /* tcprules_fdm is a filehandle to this process, which we can pipe rules into */
+ tcprules_fdm = pim[1]; close(pim[0]);
+
return(pid);
}
#endif /* POP_AUTH_OPEN_RELAY */
@@ -2681,7 +2683,7 @@
umask(VPOPMAIL_TCPRULES_UMASK);
/* open up a tcprules task, and leave it sitting waiting for the
- * new set of rules to be piped in (via the filehandle "fdm")
+ * new set of rules to be piped in (via the filehandle "tcprules_fdm")
*/
if ((pid = tcprules_open()) < 0) return(-1);
@@ -2692,7 +2694,7 @@
if ( fs != NULL ) {
/* copy the contents of the current tcp.smtp file into the tcprules pipe */
while ( fgets(tmpbuf1, sizeof(tmpbuf1), fs ) != NULL ) {
- write(fdm,tmpbuf1, strlen(tmpbuf1));
+ write(tcprules_fdm,tmpbuf1, strlen(tmpbuf1));
}
fclose(fs);
}
@@ -2701,7 +2703,7 @@
/* suck out a list of ips stored in the 'relay' table
* and write these into 'tcp.smtp' format for the tcprules pipe
*/
- vupdate_rules(fdm);
+ vupdate_rules(tcprules_fdm);
#else
@@ -2720,7 +2722,7 @@
tmpstr = strtok( tmpbuf2, "\t");
strncat(tmpstr, "\n", sizeof(tmpstr)-strlen(tmpstr)-1);
/* write the line out to the tcprules pipe */
- write(fdm,tmpstr, strlen(tmpstr));
+ write(tcprules_fdm,tmpstr, strlen(tmpstr));
}
fclose(fs);
}
@@ -2729,15 +2731,18 @@
/* close the pipe to the tcprules process. This will cause
* tcprules to generate a new tcp.smtp.cdb file
*/
- close(fdm);
+ close(tcprules_fdm);
/* wait untill tcprules finishes so we don't have zombies */
while(wait(&wstat)!= pid);
- /* unlink the temp file in case tcprules has an error */
- if ( unlink(relay_template) == -1 ) {
- return(-1);
- }
+ /* if tcprules encounters an error, then the tempfile will be
+ * left behind on the disk. We dont want this because we could
+ * possibly end up with a large number of these files cluttering
+ * the directory. Therefore we will use unlink now to make
+ * sure to zap the temp file if it still exists
+ */
+ unlink(relay_tempfile);
/* correctly set the ownership of the tcp.smtp.cdb file */
snprintf(tmpbuf1, sizeof(tmpbuf1), "%s.cdb", TCP_FILE);
