From: Calin A. C. <ca...@aj...> - 2004-01-06 21:04:00
Attachments:
smartd.c.patch
|
Hi all, I am new to the list, but I have a comment about smartd and a possible bugfix. I noticed that the way the email sending works in smartd.c could cause problems on certain platforms when sending email to multiple addresses. From the smartd.1 manpage: To send email to more than one user, please use the following "comma separated" form for the address: user1@add1,user2@add2,...,userN@addN(with no spaces). So, smartd expects email addresses as a comma-delimited list.. fair enough. However, this exact string is then passed to the mailer program in smartd.c line 535: snprintf(command, 2048, "$SMARTD_MAILER -s '%s' %s 2>&1 << \"ENDMAIL\"\n" "This email was generated by the smartd daemon running on host:\n" "%s\n" "in the domain:\n" "%s\n\n" "The following warning/error was logged by the smartd daemon:\n" "%s\n\n" "For details see the SYSLOG (default: /var/log/messages) for host:\n" "%s\n\n" "%s%s%s" "ENDMAIL\n", subject, address, hostname, domainname, message, hostname, further, original, additional); The char * 'address' string comes directly from the -m directive in the smartd.conf file. Thus, the $SMARTD_MAILER program will be passed that string verbatim. In the current implementation of smartd.c, this means a comma-separated list. This MAY work, but is not guaranteed to work with all mailer programs. The proof is the manpage. If you look at the manpage for mail or sendmail, both expect that multiple recepients be separate args on the command line, and not comma-delimited. The fact that comma-separated email addresses happens to work in most versions of sendmail (esp. postfix) is a pure coincidence. Not all versions of sendmail guarantee this. Anyway, how do we handle this? Well, I added about 4 lines of code that just takes the string from the config file, and replaces commas with spaces. This is sufficient to pass to the shell in the system() call and have it put each recipient address as a separate arg. Attached is my patch to fix smartd.c and have it pass space-separates email addresses (as separate args) to the SMARTD_MAILER program.... Please let me know what you guys think about using this patch and putting its changes in the official release of smartmontools? -Calin |
From: Bruce A. <ba...@gr...> - 2004-01-07 02:33:15
|
Hi Calin, Thanks for your note. I have always used comma separated mailing lists on the command line, and since it has always worked I simply assumed that it was correct! I'm copying this mail to the other developers -- unless someone points out a problem, after I've read the mail/mailx documentation, we'll apply your patch or something equivalent. Ed, Guido, Erik, Stanlislav, Fr=E9d=E9ric, do you agree with Calin? Cheers, =09Bruce On Tue, 6 Jan 2004, Calin A. Culianu wrote: >=20 > Hi all, >=20 > I am new to the list, but I have a comment about smartd and a possible > bugfix. >=20 > I noticed that the way the email sending works in smartd.c could cause > problems on certain platforms when sending email to multiple addresses. >=20 > From the smartd.1 manpage: >=20 > To send email to more than one user, please use the following "comma > separated" form for the address: >=20 > user1@add1,user2@add2,...,userN@addN(with no spaces). >=20 > So, smartd expects email addresses as a comma-delimited list.. fair > enough. >=20 > However, this exact string is then passed to the mailer program in > smartd.c line 535: >=20 > snprintf(command, 2048, > "$SMARTD_MAILER -s '%s' %s 2>&1 << \"ENDMAIL\"\n" > "This email was generated by the smartd daemon running on ho= st:\n" > "%s\n" > "in the domain:\n" > "%s\n\n" > "The following warning/error was logged by the smartd daemon= :\n" > "%s\n\n" > "For details see the SYSLOG (default: /var/log/messages) for= host:\n" > "%s\n\n" > "%s%s%s" > "ENDMAIL\n", > subject, address, hostname, domainname, message, hostname, fur= ther, original, additional); >=20 > The char * 'address' string comes directly from the -m directive in the > smartd.conf file. Thus, the $SMARTD_MAILER program will be passed that > string verbatim. >=20 > In the current implementation of smartd.c, this means a comma-separated > list. >=20 > This MAY work, but is not guaranteed to work with all mailer programs. >=20 > The proof is the manpage. If you look at the manpage for mail or > sendmail, both expect that multiple recepients be separate args on the > command line, and not comma-delimited. >=20 > The fact that comma-separated email addresses happens to work in most > versions of sendmail (esp. postfix) is a pure coincidence. >=20 > Not all versions of sendmail guarantee this. >=20 > Anyway, how do we handle this? Well, I added about 4 lines of code that > just takes the string from the config file, and replaces commas with > spaces. This is sufficient to pass to the shell in the system() call and > have it put each recipient address as a separate arg. >=20 > Attached is my patch to fix smartd.c and have it pass space-separates > email addresses (as separate args) to the SMARTD_MAILER program.... >=20 > Please let me know what you guys think about using this patch and putting > its changes in the official release of smartmontools? >=20 > -Calin >=20 |
From: Calin A. C. <ca...@aj...> - 2004-01-07 16:15:03
|
On Tue, 6 Jan 2004, Bruce Allen wrote: > Hi Calin, > > Thanks for your note. I have always used comma separated mailing lists o= n > the command line, and since it has always worked I simply assumed that it > was correct! Yeah, and as it turns out in like 95% of the MTA implementations out there, they are ok with an MUA giving them a comma-delimited list. In fact they try real hard to be compatible with as many funky MUAs as possible. But my argument was that we should just do the thing that works on 100% of MUA/MTA combinations, and I am glad you agree.. :) > > I'm copying this mail to the other developers -- unless someone points ou= t > a problem, after I've read the mail/mailx documentation, we'll apply your > patch or something equivalent. > > Ed, Guido, Erik, Stanlislav, Fr=E9d=E9ric, do you agree with Calin? Cool.. I await their reply. Thanks! FYI reading the mailx sourecode (its really a well written program that's easy to read) reveals that mailx's internal algorithm knows nothing about comma-delimited lists, but it does support multiple recipients via separate args on the command line. When we pass "user1@add1,user2@add2" etc to mailx, mailx internally treats that as one address, and it then proceeds to call sendmail passing it that string as 1 arg on the command line. It's actually postfix's sendmail that's doing the right thing and realizing that commas means more than 1 address. I am not sure if the original sendmail is that smart, and certainly one could imagine a more naive implementation of sendmail not supporting this particular MUA quirk.. that was my concern and my motivation for suggesting this change.... :) Cheers, -Calin |
From: Bruce A. <ba...@gr...> - 2004-01-07 16:22:26
|
> > Thanks for your note. I have always used comma separated mailing lists on > > the command line, and since it has always worked I simply assumed that it > > was correct! > > Yeah, and as it turns out in like 95% of the MTA implementations out > there, they are ok with an MUA giving them a comma-delimited list. > In fact they try real hard to be compatible with as many funky MUAs as > possible. But my argument was that we should just do the thing that > works on 100% of MUA/MTA combinations, and I am glad you agree.. :) I'm grateful that you took the trouble to point this out. > FYI reading the mailx sourecode (its really a well written program > that's easy to read) reveals that mailx's internal algorithm knows > nothing about comma-delimited lists, but it does support multiple > recipients via separate args on the command line. When we pass > "user1@add1,user2@add2" etc to mailx, mailx internally treats that as > one address, and it then proceeds to call sendmail passing it that > string as 1 arg on the command line. > > It's actually postfix's sendmail that's doing the right thing and > realizing that commas means more than 1 address. Interesting. So it's really an accident that comma-separated mail lists work at all. > I am not sure if the original sendmail is that smart, and certainly > one could imagine a more naive implementation of sendmail not > supporting this particular MUA quirk.. that was my concern and my > motivation for suggesting this change.... :) You bet. I've just finished making some mods to the smartd source and documentation to fix this. It's slightly more elaborate than what you did, mostly because (for my own reasons) I want to store the address list in comma-separated form as the user gave it, then only transform (a copy of) it when needed for mailing. Bruce |
From: Calin A. C. <ca...@aj...> - 2004-01-07 17:12:05
|
On Wed, 7 Jan 2004, Bruce Allen wrote: > You bet. I've just finished making some mods to the smartd source and > documentation to fix this. It's slightly more elaborate than what you > did, mostly because (for my own reasons) I want to store the address list > in comma-separated form as the user gave it, then only transform (a copy > of) it when needed for mailing. Yeah, I was thinking that maybe the right place to move the changes might be in the MailWarning() function. I guess you copy the address string param to a new buffer, transform it to space-delimited, then proceed to pass it to the mailer program? I guess the only remaining issue with the changes I proposed is that they might break existing shell scripts that are run using the exec option to -M.. since I could conceive of a sysadmin out there who may have written custom scripts that interact with smartd and expect 1 comma-delimited arg for address, and now his scripts are getting multiple args. His scripts may not crash, but if he actually wanted to process the addresses in his script logic (rather than passing them verbatim to the mailer program as the Example* scripts do) they will probably only acknowledge the first arg (only 1 email address when there could be more than 1)... However I would argue that in principle having multiple args is still the way to go as its more in the style of shell programming -- its slightly more convenient in shell programming to loop over multipls args rather than explicitly split them on comma.. (even the SMARTD_ADDRESS environment variable is also slightly easier to do a 'for' loop over in shell if it's space-delimited rather than comma-delimited)... So, in summary, the issue still remains that there is a possibility that existing scripts out there could still break slightly with this change.. It would be unfortunate if this change to smartd, inspired by idealism, actually ends up breaking more systems than is fixes... :) what do you think? Cheers, -Calin |
From: Bruce A. <ba...@gr...> - 2004-01-07 17:22:09
|
> > You bet. I've just finished making some mods to the smartd source and > > documentation to fix this. It's slightly more elaborate than what you > > did, mostly because (for my own reasons) I want to store the address list > > in comma-separated form as the user gave it, then only transform (a copy > > of) it when needed for mailing. > > Yeah, I was thinking that maybe the right place to move the changes might > be in the MailWarning() function. I guess you copy the address string > param to a new buffer, transform it to space-delimited, then proceed to > pass it to the mailer program? Right. > I guess the only remaining issue with the changes I proposed is that they > might break existing shell scripts that are run using the exec option to > -M.. since I could conceive of a sysadmin out there who may have written > custom scripts that interact with smartd and expect 1 comma-delimited arg > for address, and now his scripts are getting multiple args. His scripts > may not crash, but if he actually wanted to process the addresses in his > script logic (rather than passing them verbatim to the mailer program as > the Example* scripts do) they will probably only acknowledge the first arg > (only 1 email address when there could be more than 1)... I've modified (very slightly) the documentation and example scripts to make it clear that the addresses are passed 'space delimited'. Likewise the exported environment variable SMARTD_ADDRESS is now space-delimited; user scripts may need to surround it with double quotes. > However I would argue that in principle having multiple args is still the > way to go as its more in the style of shell programming -- its slightly > more convenient in shell programming to loop over multipls args rather > than explicitly split them on comma.. (even the SMARTD_ADDRESS environment > variable is also slightly easier to do a 'for' loop over in shell if it's > space-delimited rather than comma-delimited)... Agreed. > So, in summary, the issue still remains that there is a possibility that > existing scripts out there could still break slightly with this change.. > It would be unfortunate if this change to smartd, inspired by idealism, > actually ends up breaking more systems than is fixes... :) what do you > think? In this case, I don't think it will break too much. 5.27/28 have also made some other changes to the mailing. In particular, if the mail agent or the mailing script writes to stdout or stderr, a snippet of that is logged to SYSLOG, to help track down problems with mail. Previously stderr and stdout were flushed to /dev/null. Calin, I'd like you to test/check the latest code. I'm going to send you a source tarball in a separate email. Cheers, Bruce |
From: Calin A. C. <ca...@aj...> - 2004-01-07 17:21:54
|
On Wed, 7 Jan 2004, Bruce Allen wrote: > > Interesting. So it's really an accident that comma-separated mail lists > work at all. > Actually, to be honest, most sendmail programs, even the feature-limited ssmtp try and parse ',' on the command line... and I can't personally find even 1 sendmail program that doesn't do that.. so I am not sure what the official 'right thing' to do is, but I do know that _all_ manpages out there suggest multiple args for multiple recipients, so to me that indicates that the comma-delimited thing may be just a hidden feature. Also, mailx doesn't parse ',', so that alone might indicate it isn't 100% kosher to expect mailers to parse ','...? -Calin |