Re: MySQL stored-procedure support for Postfix 3.2
On 12/25/2016 01:17 AM, Wietse Venema wrote: > John Fawcett: >> Revised patch to improve error reporting when no result set containing >> data is returned > This code is now part of postfix-3.2-20161224-nonprod, slightly > edited to simplify error handling. I would be interested to hear > if it still works with queries that don't call a stored procedure. > > Wietse Wietse your release has literally crossed with my testing and preparation of a revised fix. I will post it now and hope you can use that. I am sorry for your wasted time. I have done comparisons on this version to unpatched Postfix. I confirm as you picked up that the old patch gives incorrect error messages (though the results are consistent) compared to unpatched Postfix, but in various situations. I had to change the logic. John
Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup
On 12/18/2016 09:38 PM, John Fawcett wrote: > On 12/18/2016 02:09 AM, Wietse Venema wrote: >> What if Postfix made an old-style query? I think it should just >> report the old-style error here. >> >> Wietse > I agree. It might be as simple as changing > > msg_warn("dict_mysql: stored procedure did not return any result set > containing data"); > > to > > msg_warn("mysql query failed: %s", mysql_error(host->db)); > > > But I think I need to do some tests with unmodified postfix and then compare > behaviour to the patched version to make sure the warnings are really the > same. > > John > I have revised the patch and changed the logic. The error messages are now in line with the exisiting behaviour for queries that return results, queries that return no results and for queries that have errors. There is one minor difference in error messages in the case of a query that contains no SELECT. The result returned by mysql is indistinguishable from a stored procedure that contains no SELECT and as I am explicitly setting errno to ENOSUP, the message given for the query would now be: postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf postmap: warning: mysql query failed: postmap: fatal: table mysql:/etc/postfix/test_update.cf: query error: Operation not supported instead of the existing postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf postmap: warning: mysql query failed: postmap: fatal: table mysql:/etc/postfix/test_update.cf: query error: Success Queries containing errors continue to have errno 0 as currently postmap -q erba.tv mysql:/etc/postfix/test_query_error.cf postmap: warning: mysql query failed: Unknown column 'domains' in 'field list' postmap: fatal: table mysql:/etc/postfix/test_query_error.cf: query error: Success though I would opt to set it in this case too. This version of the patch includes some documentation updates to clarify that the stored procedure must execute exactly one SELECT statement. The code has been modified for the revised error reporting and readability. --- postfix-3.2-20161204_orig/proto/mysql_table2016-10-01 15:44:52.0 +0200 +++ postfix-3.2-20161204/proto/mysql_table2016-12-25 09:04:09.230528031 +0100 @@ -289,6 +289,52 @@ #certificate. # .sp #This parameter is available with Postfix 2.11 and later. +# USING MYSQL STORED PROCEDURES +# .ad +# .fi +#With Postfix 3.2 it is possible to call a stored procedure +#instead of using a SELECT statement in the query, e.g. +# +# .nf +#query = CALL lookup('%s') +# .fi +# +#The preivously described '%' expansions can be used in the +#parameter(s) to the stored procedure. +# +#The stored procedure must return data with a single result +#set, that is the stored procedure must execute exactly one +#SELECT statement on every code path. If you have complex +#logic and for some paths you want to return no result you +#will need to include a SELECT statement that returns no +#rows. One example of a query that returns no rows is +# +# .nf +#SELECT 1 FROM DUAL WHERE FALSE +# .fi +# +#but you may use your own query. +# +#Stored procedures that return multiple result sets containing +#data, that is stored procedures that execute multiple SELECT +#statements, are not supported. Stored procedures in +#mysql produce an additional result set without data which +#indicates the final status of the stored procedure. If this +#final status is an error then any previous returned data +#will not be used. +# +# The following is an example of a stored procedure returning +# a single result set containing data: +# +# .nf +#CREATE [DEFINER=`user`@`host`] PROCEDURE +#`lookup`(IN `param` VARCHAR(255)) +#READS SQL DATA +#SQL SECURITY INVOKER +#BEGIN +#select goto from alias where address=param; +#END +# .fi # OBSOLETE QUERY INTERFACE # .ad # .fi diff -ur postfix-3.2-20161204_orig/src/global/dict_mysql.c postfix-3.2-20161204/src/global/dict_mysql.c --- postfix-3.2-20161204_orig/src/global/dict_mysql.c2016-09-25 17:27:11.0 +0200 +++ postfix-3.2-20161204/src/global/dict_mysql.c2016-12-24 19:56:24.967466042 +0100 @@ -187,6 +187,7 @@ #include #include #include +#include #ifdef STRCASECMP_IN_STRINGS_H #include @@ -519,6 +520,10 @@ { HOST *host; MYSQL_RES *res = 0; +MYSQL_RES *temp_res = 0; +int status=0; +int num_result_sets=0; +int query_error=0; while ((host = dict_mysql_get_active(dict_mysql)) != NULL) { #if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 4 @@ -536,9 +541,56 @@ #endif if (!(mysql_query(host->db, vstring_str(query { -if ((res = mysql_store_result(host->db)) == 0) { +query_error = 0; +num_result_sets = 0; +res = 0; +do { +temp_res = mysql_store_result(host->db); +/* did statement ret
Re: MySQL stored-procedure support for Postfix 3.2
On 12/25/2016 09:30 AM, John Fawcett wrote: > On 12/25/2016 01:17 AM, Wietse Venema wrote: >> John Fawcett: >>> Revised patch to improve error reporting when no result set containing >>> data is returned >> This code is now part of postfix-3.2-20161224-nonprod, slightly >> edited to simplify error handling. I would be interested to hear >> if it still works with queries that don't call a stored procedure. >> >> Wietse > Wietse > > your release has literally crossed with my testing and preparation of a > revised fix. I will post it now and hope you can use that. I am sorry > for your wasted time. I have done comparisons on this version to > unpatched Postfix. I confirm as you picked up that the old patch gives > incorrect error messages (though the results are consistent) compared to > unpatched Postfix, but in various situations. I had to change the logic. > > John > > I downloaded and tested your version. It is almost functionally equivalent to my new version (for queries and stored procedures). Your version like my new one has the changed errno for queries that have no select postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf postmap: warning: mysql query failed: postmap: fatal: table mysql:/etc/postfix/test_update.cf: query error: Operation not supported instead of unpatched postfix postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf postmap: warning: mysql query failed: postmap: fatal: table mysql:/etc/postfix/test_update.cf: query error: Success The only difference I found functionally was in the case of more than two result sets. My new version prints the error message only once postmap -q j...@erba.tv mysql:/etc/postfix/test_call_res2.cf postmap: warning: mysql query failed: postmap: warning: dict_mysql: multiple result sets returning data are not supported postmap: fatal: table mysql:/etc/postfix/test_call_res2.cf: query error: Operation not supported instead of this (my old version and your released version) postmap -q j...@erba.tv mysql:/etc/postfix/test_call_res2.cf postmap: warning: mysql query failed: multiple result sets returning data are not supported postmap: warning: mysql query failed: multiple result sets returning data are not supported postmap: fatal: table mysql:/etc/postfix/test_call_res2.cf: query error: Operation not supported So I confirm your version, unless you think my latest version is more readable. John
Re: Postfix delivery problem
> Georg > > I don't think there is enough evidence at the moment to say with > certainty that any change in glibc has introduced the problem, since you > were using that for a while now without seeing issues. > > I'd still be interested in knowing what output the test program gives on > the affected server. > > best wishes to you too. > > John oh, right, the test program. me@dukun ~ > ./test2 Not found me@dukun ~ > echo $? 1 i hope this is the information you looked for? i didnt think about it too much at the moment… georg
Re: Postfix delivery problem
On 12/25/2016 11:10 AM, G. Schlisio wrote: >> Georg >> >> I don't think there is enough evidence at the moment to say with >> certainty that any change in glibc has introduced the problem, since you >> were using that for a while now without seeing issues. >> >> I'd still be interested in knowing what output the test program gives on >> the affected server. >> >> best wishes to you too. >> >> John > oh, right, the test program. > > me@dukun ~ > ./test2 > Not found > me@dukun ~ > echo $? > 1 > > i hope this is the information you looked for? i didnt think about it > too much at the moment… > georg thanks. That result could cause some headaches. To be consistent with the trace posted at http://termbin.com/ram9 and specifically these lines Dez 23 12:21:44 dukun postfix/local[4396]: deliver_dotforward[3]: local recip a...@dukun.de exten deliver a...@dukun.de exp_from Dez 23 12:21:44 dukun postfix/local[4396]: warning: error looking up passwd info for : Invalid argument the expected result from the test program is a message about invalid argument. You might try to see if postfix is still giving you the same trace as before for the user (with the forward_path workaround removed) or if now it is working. That postfix gets one response and the test program a different response on the same server without changing anything else (like nsswitch.conf) or restarting the server etc. is puzzling. I suppose the test you tried in postfix was with that exact same user and that is not an anonymization of a longer username. Whatever the case on your server, I think this issue is going to need to be addressed, presumably in archlinux or glibc since I found a reproducable case of your original problem on a stock archlinux install with glibc 2.24: sendmail -bv Dec 25 11:51:48 archlinux postfix/local[2974]: warning: error looking up passwd info for : Invalid argument This is reproducable with the test program (so independently of Postfix). Could you try to get this addressed otherwise more people are going to start finding problems with postfix local delivery on archlinux platform (and potentially with other platforms if the cause is within glibc itself and those platforms start to update to newer releases). I say "if the cause is within glibc" since it may be something that also depends on what nsswitch.conf configurations are being used. In the example below, the getpwnam_r routine returns a correct answer for an inexistent user for strings up to 31 chars. From 32 chars onwards instead of returning not found it retuns EINVAL (invalid argument). /test AAA Not found ./test getpwnam_r: Invalid argument John
Re: MySQL stored-procedure support for Postfix 3.2
John Fawcett: [ Charset windows-1252 converted... ] > On 12/25/2016 09:30 AM, John Fawcett wrote: > > On 12/25/2016 01:17 AM, Wietse Venema wrote: > >> John Fawcett: > >>> Revised patch to improve error reporting when no result set containing > >>> data is returned > >> This code is now part of postfix-3.2-20161224-nonprod, slightly > >> edited to simplify error handling. I would be interested to hear > >> if it still works with queries that don't call a stored procedure. > >> > >>Wietse > > Wietse > > > > your release has literally crossed with my testing and preparation of a > > revised fix. I will post it now and hope you can use that. I am sorry > > for your wasted time. I have done comparisons on this version to > > unpatched Postfix. I confirm as you picked up that the old patch gives > > incorrect error messages (though the results are consistent) compared to > > unpatched Postfix, but in various situations. I had to change the logic. > > > > John > > > > > I downloaded and tested your version. It is almost functionally > equivalent to my new version (for queries and stored procedures). > > Your version like my new one has the changed errno for queries that have > no select > > postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf > postmap: warning: mysql query failed: > postmap: fatal: table mysql:/etc/postfix/test_update.cf: query > error: Operation not supported > > instead of unpatched postfix > > postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf > postmap: warning: mysql query failed: > postmap: fatal: table mysql:/etc/postfix/test_update.cf: query > error: Success I think that Postfix should not report 'Success' for a failed query. EOPNOTSUPP or EINVAL would be more appropriate. > The only difference I found functionally was in the case of more than > two result sets. My new version prints the error message only once > > postmap -q j...@erba.tv mysql:/etc/postfix/test_call_res2.cf > postmap: warning: mysql query failed: > postmap: warning: dict_mysql: multiple result sets returning data are > not supported > postmap: fatal: table mysql:/etc/postfix/test_call_res2.cf: query error: > Operation not supported This can be fixed with inserting a 'break' after every error, but perhaps this code should not be looping over result sets in the first place. There are only two iterations, and each iteration does very different things. I'll diff your new patch against the previous version and see what changes you made.
Re: Postfix delivery problem
John Fawcett: > for an inexistent user for strings up to 31 chars. From 32 chars onwards > instead of returning not found it retuns EINVAL (invalid argument). > > ./test AAA > Not found > ./test > getpwnam_r: Invalid argument Perhaps they want programs to call sysconf(_SC_LOGIN_NAME_MAX) (note that the result includes the null terminator). Wietse #include #include #include #include #include #include int main(int argc, char *argv[]) { struct passwd pwd; struct passwd *result; char *buf; size_t bufsize; int s; if (argc != 2) { fprintf(stderr, "Usage: %s username\n", argv[0]); exit(EXIT_FAILURE); } bufsize = sysconf(_SC_GETPW_R_SIZE_MAX); if (bufsize == -1) /* Value was indeterminate */ bufsize = 16384;/* Should be more than enough */ buf = malloc(bufsize); if (buf == NULL) { perror("malloc"); exit(EXIT_FAILURE); } /* _SC_LOGIN_NAME_MAX includes the null terminator */ if (strlen(argv[1]) >= sysconf(_SC_LOGIN_NAME_MAX)) { fprintf(stderr, "warning: name exceeds _SC_LOGIN_NAME_MAX\n"); result = 0; s = 0; } else { s = getpwnam_r(argv[1], &pwd, buf, bufsize, &result); } if (result == NULL) { if (s == 0) printf("Not found\n"); else { errno = s; perror("getpwnam_r"); } exit(EXIT_FAILURE); } printf("Name: %s; UID: %ld\n", pwd.pw_gecos, (long) pwd.pw_uid); free(buf); exit(EXIT_SUCCESS); }
Re: MySQL stored-procedure support for Postfix 3.2
On 12/25/2016 04:46 PM, Wietse Venema wrote: > John Fawcett: > [ Charset windows-1252 converted... ] >> On 12/25/2016 09:30 AM, John Fawcett wrote: >>> On 12/25/2016 01:17 AM, Wietse Venema wrote: John Fawcett: > Revised patch to improve error reporting when no result set containing > data is returned This code is now part of postfix-3.2-20161224-nonprod, slightly edited to simplify error handling. I would be interested to hear if it still works with queries that don't call a stored procedure. Wietse >>> Wietse >>> >>> your release has literally crossed with my testing and preparation of a >>> revised fix. I will post it now and hope you can use that. I am sorry >>> for your wasted time. I have done comparisons on this version to >>> unpatched Postfix. I confirm as you picked up that the old patch gives >>> incorrect error messages (though the results are consistent) compared to >>> unpatched Postfix, but in various situations. I had to change the logic. >>> >>> John >>> >>> >> I downloaded and tested your version. It is almost functionally >> equivalent to my new version (for queries and stored procedures). >> >> Your version like my new one has the changed errno for queries that have >> no select >> >> postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf >> postmap: warning: mysql query failed: >> postmap: fatal: table mysql:/etc/postfix/test_update.cf: query >> error: Operation not supported >> >> instead of unpatched postfix >> >> postmap -q j...@erba.tv mysql:/etc/postfix/test_update.cf >> postmap: warning: mysql query failed: >> postmap: fatal: table mysql:/etc/postfix/test_update.cf: query >> error: Success > I think that Postfix should not report 'Success' for a failed query. > EOPNOTSUPP or EINVAL would be more appropriate. > >> The only difference I found functionally was in the case of more than >> two result sets. My new version prints the error message only once >> >> postmap -q j...@erba.tv mysql:/etc/postfix/test_call_res2.cf >> postmap: warning: mysql query failed: >> postmap: warning: dict_mysql: multiple result sets returning data are >> not supported >> postmap: fatal: table mysql:/etc/postfix/test_call_res2.cf: query error: >> Operation not supported > This can be fixed with inserting a 'break' after every error, but > perhaps this code should not be looping over result sets in the > first place. There are only two iterations, and each iteration does > very different things. > > I'll diff your new patch against the previous version and see what > changes you made. if you break out of the loop without reading all the result data you will get the following mysql error on the next query. Commands out of sync; you can't run this command now John John
Re: Postfix delivery problem
On 12/25/2016 05:12 PM, Wietse Venema wrote: > John Fawcett: >> for an inexistent user for strings up to 31 chars. From 32 chars onwards >> instead of returning not found it retuns EINVAL (invalid argument). >> >> ./test AAA >> Not found >> ./test >> getpwnam_r: Invalid argument > Perhaps they want programs to call sysconf(_SC_LOGIN_NAME_MAX) > (note that the result includes the null terminator). > > Wietse > > #include > #include > #include > #include > #include > #include > > int main(int argc, char *argv[]) > { > struct passwd pwd; > struct passwd *result; > char *buf; > size_t bufsize; > int s; > > if (argc != 2) { > fprintf(stderr, "Usage: %s username\n", argv[0]); > exit(EXIT_FAILURE); > } > bufsize = sysconf(_SC_GETPW_R_SIZE_MAX); > if (bufsize == -1) /* Value was indeterminate */ > bufsize = 16384;/* Should be more than enough > */ > > buf = malloc(bufsize); > if (buf == NULL) { > perror("malloc"); > exit(EXIT_FAILURE); > } > /* _SC_LOGIN_NAME_MAX includes the null terminator */ > if (strlen(argv[1]) >= sysconf(_SC_LOGIN_NAME_MAX)) { > fprintf(stderr, "warning: name exceeds _SC_LOGIN_NAME_MAX\n"); > result = 0; > s = 0; > } else { > s = getpwnam_r(argv[1], &pwd, buf, bufsize, &result); > } > if (result == NULL) { > if (s == 0) > printf("Not found\n"); > else { > errno = s; > perror("getpwnam_r"); > } > exit(EXIT_FAILURE); > } > printf("Name: %s; UID: %ld\n", pwd.pw_gecos, (long) pwd.pw_uid); > free(buf); > exit(EXIT_SUCCESS); > } I tried that on archlinux. The above program still produces EINVAL for login names between 32 and 255 inclusive. _SC_LOGIN_NAME_MAX is 256 on that platform. John
Re: Postfix delivery problem
> > I tried that on archlinux. The above program still produces EINVAL for > login names between 32 and 255 inclusive. > > _SC_LOGIN_NAME_MAX is 256 on that platform. > > John > hi, earlier i tried with literal "AA", which was probably not what you meant. it produced a "not found". using an affected user name yields (with both your programs): getpwnam_r: Invalid argument while an existing shell account yields its UID: Name: ; UID: 1001 for me its not clear whats the difference between a literal string and an existing username not in passwd (why one produces an error and one doesnt). sorry for not reading carefully earlier… georg
Re: Postfix delivery problem
On 12/25/2016 06:30 PM, G. Schlisio wrote: >> I tried that on archlinux. The above program still produces EINVAL for >> login names between 32 and 255 inclusive. >> >> _SC_LOGIN_NAME_MAX is 256 on that platform. >> >> John >> > hi, > > earlier i tried with literal "AA", which was probably not what you > meant. > it produced a "not found". > using an affected user name yields (with both your programs): > getpwnam_r: Invalid argument > while an existing shell account yields its UID: > Name: ; UID: 1001 > > for me its not clear whats the difference between a literal string > and an existing username not in passwd (why one produces an error and > one doesnt). > > sorry for not reading carefully earlier… > > georg Hi Georg thanks for that, so at least we have consistent behaviour which is good. I had got the from your logging without realizing it was anonymized. Now the problem to solve is why the user names you are testing with give the invalid argument. One reason would be if they are longer than 31 chars. But if they are not then there is something else, for example do they contain a dot or some other character that archlinux does not accept in user names? At this point I suspect that your system is providing EINVAL for any name that could not be a valid local username (with one exception that it allows uppercase which is not valid in usernames). According to the archlinux useradd manual: "Usernames must start with a lower case letter or an underscore, followed by lower case letters, digits, underscores, or dashes. They can end with a dollar sign. In regular expression terms: [a-z_][a-z0-9_-]*[$]? Usernames may only be up to 32 characters long." This is a problem because the getpwnam_r routine on archlinux is deviating from the specifications by not always returning 0 for users that are not found. This should be addressed in archlinux or upstream in glibc. Can you take it back to the archlinux list and see if you can get this addressed? John
Re: Postfix delivery problem
On 12/25/2016 07:40 PM, John Fawcett wrote: > Hi Georg > thanks for that, so at least we have consistent behaviour which is good. > I had got the from your logging without realizing it was > anonymized. > > Now the problem to solve is why the user names you are testing with give > the invalid argument. One reason would be if they are longer than 31 > chars. But if they are not then there is something else, for example do > they contain a dot or some other character that archlinux does not > accept in user names? > > At this point I suspect that your system is providing EINVAL for any > name that could not be a valid local username (with one exception that > it allows uppercase which is not valid in usernames). According to the > archlinux useradd manual: > >"Usernames must start with a lower case letter or an underscore, > followed by lower case letters, digits, > underscores, or dashes. They can end with a dollar sign. In > regular expression terms: > [a-z_][a-z0-9_-]*[$]? > >Usernames may only be up to 32 characters long." > > This is a problem because the getpwnam_r routine on archlinux is > deviating from the specifications by not always returning 0 for users > that are not found. This should be addressed in archlinux or upstream in > glibc. Can you take it back to the archlinux list and see if you can get > this addressed? > > John > I managed to find where this is happening. It is not in glibc but in systemd. If your /etc/nsswitch.conf has something like this: passwd: compat mymachines systemd then the routines that are being used are systemd ones. The checks being done are here in the function valid_user_group_name: https://github.com/systemd/systemd/blob/master/src/basic/user-util.c and in the case that those checks fail then _nss_systemd_getpwnam_r from systemd libraries returns EINVAL https://github.com/systemd/systemd/blob/master/src/nss-systemd/nss-systemd.c if (!valid_user_group_name(name)) { r = -EINVAL; goto fail; } ... fail: *errnop = -r; return NSS_STATUS_UNAVAIL; So the problem is that systemd version of getpwnam_r is deviating from the standard of returning 0 for not found users. Either the systemd library module or the nsswitch.conf is probably what changed during your upgrade. John
Re: MySQL stored-procedure support for Postfix 3.2
John Fawcett: > > I'll diff your new patch against the previous version and see what > > changes you made. > > if you break out of the loop without reading all the result data you > will get the following mysql error on the next query. > > Commands out of sync; you can't run this command now Noted. Meanwhile I have separated 1) the code that handles the result set with the lookup result, from 2) the code that handles all other result sets. Without interleaving 1) and 2), the code becomes easier to follow. It's uploaded as postfix-3.2-20161225-nonprod; please check it out. Wietse
Re: MySQL stored-procedure support for Postfix 3.2
On 12/25/2016 08:45 PM, Wietse Venema wrote: > John Fawcett: >>> I'll diff your new patch against the previous version and see what >>> changes you made. >> if you break out of the loop without reading all the result data you >> will get the following mysql error on the next query. >> >> Commands out of sync; you can't run this command now > Noted. Meanwhile I have separated 1) the code that handles the > result set with the lookup result, from 2) the code that handles > all other result sets. > > Without interleaving 1) and 2), the code becomes easier to follow. > It's uploaded as postfix-3.2-20161225-nonprod; please check it out. > > Wietse I confirm it behaves as expected on positive and negative cases. The code is indeed much easier to follow. John
Re: MySQL stored-procedure support for Postfix 3.2
John Fawcett: > > Noted. Meanwhile I have separated 1) the code that handles the > > result set with the lookup result, from 2) the code that handles > > all other result sets. > > > > Without interleaving 1) and 2), the code becomes easier to follow. > > It's uploaded as postfix-3.2-20161225-nonprod; please check it out. > > > I confirm it behaves as expected on positive and negative cases. The > code is indeed much easier to follow. I see room for two improvements: - Don't loop on mysql_next_result() if that function returns an error, to avoid going into an infinite loop. - Do call mysql_next_result() after mysql_store_result() etc. error just to be sure that we stay in sync. This won't log any Postfix warnings when the query_error flag was already raised. Wietse