Quantcast

Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
33 messages Options
12
jl
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

jl

Hi guys,

think I found a bug using Ubuntu 16.04, can you confirm this?

what I get in the log:
Jul 3 01:21:40 postfix/cleanup[10334]: warning: mysql query failed: Commands out of sync; you can't run this command now
Jul 3 01:21:40 postfix/cleanup[10334]: warning: mysql:/etc/postfix/sql/aliases.cf lookup error for "[hidden email]"
Jul 3 01:21:40 postfix/cleanup[10334]: warning: EF7AE1C0FCE: virtual_alias_maps map lookup problem for [hidden email] -- message n$

Configuration:
virtual_alias_maps = mysql:/etc/postfix/sql/aliases.cf

and the query in that file:
query = CALL get_aliases_destinations('%u', '%d')

the stored procedure in the database could be as simple as
CREATE PROCEDURE `get_aliases_destinations`(IN user VARCHAR(64), IN domain VARCHAR(255))
BEGIN
 SELECT CONCAT(destination_username, '@', destination_domain) AS destinations FROM aliases WHERE source_username = user AND source_domain = domain AND enabled = true;
END

The bare select itself as query does work fine.
The crucial thing is that [hidden email] already is the alias destination, so the first query worked fine. In fact postalias -q .... returns valid results.
I believe this occurs because of the mysql library returning an second empty result set which is not removed/cleared by postfix.
See this discussion which describes a similar issue:
http://stackoverflow.com/questions/6583020/mysql-stored-procedure-caused-commands-out-of-sync
I'm not familiar with the postfix sources but I think a possible fix could be inserted into the plmysql_query function in dict_mysql.c similar to what is discussed in the link and a dozen similar mysql api issues on the web.

 


smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

John Fawcett
On 07/03/2016 05:35 PM, Joel Linn wrote:

Hi guys,

think I found a bug using Ubuntu 16.04, can you confirm this?

...
Hi
it's not actually a bug. Postfix does not support mysql stored procedures.

This was discussed here back in 2008:

http://osdir.com/ml/mail.postfix.devel/2008-02/msg00021.html

best regards
John
jl
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

jl

Why is it chosen to "not support stored procedures" instead of adding two lines of code?

Are there any security or performance implications? I don't see why the fix would not be applied.

I'am now running a query that is several hundred characters long and doesn't scale linear, just to shoehorn everything into one query; It could be done much easier and faster in a stored procedure.

 

Von: [hidden email] [mailto:[hidden email]] Im Auftrag von John Fawcett
Gesendet: Sonntag, 3. Juli 2016 18:10
An: [hidden email]
Betreff: Re: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

 

On 07/03/2016 05:35 PM, Joel Linn wrote:

Hi guys,

think I found a bug using Ubuntu 16.04, can you confirm this?

...

Hi
it's not actually a bug. Postfix does not support mysql stored procedures.

This was discussed here back in 2008:

http://osdir.com/ml/mail.postfix.devel/2008-02/msg00021.html

best regards
John


smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

Wietse Venema
Joel Linn:
> Why is it chosen to "not support stored procedures" instead of adding two
> lines of code?

The original mysql client may well have been written at a time that
stored procedures did not exist, or no API documentation existed
for how to do this correctly.

If you feel strongly about stored procedures, then you are welcome
to contribute code. If you can demonstrate that the code uses the
MySQL API correctly (instead of "there are no error messages") then
that would help getting the code accepted.

        Wietse
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

Wietse Venema
Wietse Venema:

> Joel Linn:
> > Why is it chosen to "not support stored procedures" instead of adding two
> > lines of code?
>
> The original mysql client may well have been written at a time that
> stored procedures did not exist, or no API documentation existed
> for how to do this correctly.
>
> If you feel strongly about stored procedures, then you are welcome
> to contribute code. If you can demonstrate that the code uses the
> MySQL API correctly (instead of "there are no error messages") then
> that would help getting the code accepted.

This is a pointer to MySQL documentation for multi-statement support,
which seems to be needed to implement stored-procedure support in the
Postfix MySQL client.

http://dev.mysql.com/doc/refman/5.7/en/c-api-multiple-queries.html

        Wietse
jl
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

jl

Quoting [hidden email]:

> Wietse Venema:
>> Joel Linn:
>> > Why is it chosen to "not support stored procedures" instead of adding two
>> > lines of code?
>>
>> The original mysql client may well have been written at a time that
>> stored procedures did not exist, or no API documentation existed
>> for how to do this correctly.
>>
>> If you feel strongly about stored procedures, then you are welcome
>> to contribute code. If you can demonstrate that the code uses the
>> MySQL API correctly (instead of "there are no error messages") then
>> that would help getting the code accepted.
>
> This is a pointer to MySQL documentation for multi-statement support,
> which seems to be needed to implement stored-procedure support in the
> Postfix MySQL client.
>
> http://dev.mysql.com/doc/refman/5.7/en/c-api-multiple-queries.html
>
> Wietse

Thanks for the link.
An interesting fact is that the doc says CLIENT_MULTI_RESULTS is  
default since MySQL 5.7.
And comparing my error message "Commands out of sync" to the one in  
the discussion of 2008 John Fawcett dug out
"can't return a result set in the given context", we are in fact not  
using the api as designed at the moment.
Sure its "just" an error message for both cases but the api thinks we  
can handle multiple results but in fact we can not.
Talking about multi statement support is planning one step ahead  
already, but a smart thought whatsoever, eliminating the need for  
stored procedures to some extend.

I think we could at least support multi results by enumerating over  
all but the first result (return that) and maybe throw them away,
at least issue a warning if they are additional result sets (not for  
statuses of course, they are OK if they indicate success).
This way we can indicate to the administrator a poorly designed query  
that leaks data.
Maybe even issue an error for security reasons if the results contain  
more than one result set.
Because that could lead to undefined behavior if some SELECT prior to  
the correct SELECT accidentally gives wrong data (e.g. sending mail to  
mars).

Multiple statements is another discussion, however subordinate and  
dependent on the implementation of multi results.
As far is I can see no prepared statements are used in the code so  
allowing for multiple statements is easy and shouldn't tension the  
security discussion (?),
however sql injection is easier to do if you can end a statement with ";".
But I presume the % placeholders are filtered anyway (either explicit  
or implicit) so that shouldn't be a concern.

Do you agree or do you see any drawbacks or problems?

Joel

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

Wietse Venema
[hidden email]:

>
> Quoting [hidden email]:
>
> > Wietse Venema:
> >> Joel Linn:
> >> > Why is it chosen to "not support stored procedures" instead of adding two
> >> > lines of code?
> >>
> >> The original mysql client may well have been written at a time that
> >> stored procedures did not exist, or no API documentation existed
> >> for how to do this correctly.
> >>
> >> If you feel strongly about stored procedures, then you are welcome
> >> to contribute code. If you can demonstrate that the code uses the
> >> MySQL API correctly (instead of "there are no error messages") then
> >> that would help getting the code accepted.
> >
> > This is a pointer to MySQL documentation for multi-statement support,
> > which seems to be needed to implement stored-procedure support in the
> > Postfix MySQL client.
> >
> > http://dev.mysql.com/doc/refman/5.7/en/c-api-multiple-queries.html
> >
> > Wietse
>
> Thanks for the link.
> An interesting fact is that the doc says CLIENT_MULTI_RESULTS is  
> default since MySQL 5.7.
> And comparing my error message "Commands out of sync" to the one
> in the discussion of 2008 John Fawcett dug out "can't return a
> result set in the given context", we are in fact not using the api
> as designed at the moment.  Sure its "just" an error message for
> both cases but the api thinks we can handle multiple results but
> in fact we can not.  Talking about multi statement support is
> planning one step ahead already, but a smart thought whatsoever,
> eliminating the need for stored procedures to some extend.
>
> I think we could at least support multi results by enumerating
> over all but the first result (return that) and maybe throw them
> away, at least issue a warning if they are additional result sets
> (not for statuses of course, they are OK if they indicate success).
> This way we can indicate to the administrator a poorly designed
> query that leaks data.  Maybe even issue an error for security
> reasons if the results contain more than one result set.  Because
> that could lead to undefined behavior if some SELECT prior to the
> correct SELECT accidentally gives wrong data (e.g. sending mail
> to mars).
>
> Multiple statements is another discussion, however subordinate and
> dependent on the implementation of multi results.  As far is I can
> see no prepared statements are used in the code so allowing for
> multiple statements is easy and shouldn't tension the security
> discussion (?), however sql injection is easier to do if you can
> end a statement with ";".  But I presume the % placeholders are
> filtered anyway (either explicit or implicit) so that shouldn't
> be a concern.
>
> Do you agree or do you see any drawbacks or problems?

Unfortunately, I am not familiar with MySQL APIs, and I don't have
cycles to spare.

My first priority is to ensure that existing Postfix code keeps
working as promised (i.e. Postfix does not break due to MySQL API
library changes).

If the MySQL library will send multiple results even if Postfix
does not specify CLIENT_MULTI_RESULTS, it is sufficient to report
an error when the library returns more replies than expected?

I'm not opposed to stored-procedure support, but I think it should
be implemented+documented by someone who understands the APIs, so
that would not be me.

        Wietse
jl
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

jl

Quoting [hidden email]:

> [hidden email]:
>>
>> Quoting [hidden email]:
>>
>> > Wietse Venema:
>> >> Joel Linn:
>> >> > Why is it chosen to "not support stored procedures" instead of  
>> adding two
>> >> > lines of code?
>> >>
>> >> The original mysql client may well have been written at a time that
>> >> stored procedures did not exist, or no API documentation existed
>> >> for how to do this correctly.
>> >>
>> >> If you feel strongly about stored procedures, then you are welcome
>> >> to contribute code. If you can demonstrate that the code uses the
>> >> MySQL API correctly (instead of "there are no error messages") then
>> >> that would help getting the code accepted.
>> >
>> > This is a pointer to MySQL documentation for multi-statement support,
>> > which seems to be needed to implement stored-procedure support in the
>> > Postfix MySQL client.
>> >
>> > http://dev.mysql.com/doc/refman/5.7/en/c-api-multiple-queries.html
>> >
>> > Wietse
>>
>> Thanks for the link.
>> An interesting fact is that the doc says CLIENT_MULTI_RESULTS is
>> default since MySQL 5.7.
>> And comparing my error message "Commands out of sync" to the one
>> in the discussion of 2008 John Fawcett dug out "can't return a
>> result set in the given context", we are in fact not using the api
>> as designed at the moment.  Sure its "just" an error message for
>> both cases but the api thinks we can handle multiple results but
>> in fact we can not.  Talking about multi statement support is
>> planning one step ahead already, but a smart thought whatsoever,
>> eliminating the need for stored procedures to some extend.
>>
>> I think we could at least support multi results by enumerating
>> over all but the first result (return that) and maybe throw them
>> away, at least issue a warning if they are additional result sets
>> (not for statuses of course, they are OK if they indicate success).
>> This way we can indicate to the administrator a poorly designed
>> query that leaks data.  Maybe even issue an error for security
>> reasons if the results contain more than one result set.  Because
>> that could lead to undefined behavior if some SELECT prior to the
>> correct SELECT accidentally gives wrong data (e.g. sending mail
>> to mars).
>>
>> Multiple statements is another discussion, however subordinate and
>> dependent on the implementation of multi results.  As far is I can
>> see no prepared statements are used in the code so allowing for
>> multiple statements is easy and shouldn't tension the security
>> discussion (?), however sql injection is easier to do if you can
>> end a statement with ";".  But I presume the % placeholders are
>> filtered anyway (either explicit or implicit) so that shouldn't
>> be a concern.
>>
>> Do you agree or do you see any drawbacks or problems?
>
> Unfortunately, I am not familiar with MySQL APIs, and I don't have
> cycles to spare.
>
> My first priority is to ensure that existing Postfix code keeps
> working as promised (i.e. Postfix does not break due to MySQL API
> library changes).
>
> If the MySQL library will send multiple results even if Postfix
> does not specify CLIENT_MULTI_RESULTS, it is sufficient to report
> an error when the library returns more replies than expected?
>
> I'm not opposed to stored-procedure support, but I think it should
> be implemented+documented by someone who understands the APIs, so
> that would not be me.
>
> Wietse

At the moment no flags (0) are passed to mysql_real_connect.
But if using the mysql api version 5.7 or newer the api "forces"  
CLIENT_MULTI_RESULTS.
I see no way in the api doc to disable it, e.g. using mysql_set_server_option.
So the code should no matter what cycle over the results anyway,
because if not cleared they cause later queries to fail and
we have no way to prohibit multiple results using the api afaIk.

So yes, looping results and generating an error for more than one
result is sufficient to prevent hiccups in following queries.

That bugfix however would be very close to my suggested change.
Is there someone maintaining the mysql portion of postfix,
I mean except you keeping everything running?

Joel


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

Wietse Venema
[hidden email]:
> So yes, looping results and generating an error for more than one
> result is sufficient to prevent hiccups in following queries.
>
> That bugfix however would be very close to my suggested change.
> Is there someone maintaining the mysql portion of postfix,
> I mean except you keeping everything running?

There is no maintainer, I just made little fixes over time in the
parts that have no direct effect on MySQL API calls.  If you can
come up with code to iterate over multiple replies them you are
welcome to share that. If that is all it takes to support stored
procedures, please update proto/mysql_table as well.

        Wietse
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

John Fawcett
In reply to this post by jl
On 07/04/2016 03:30 PM, [hidden email] wrote:

>
> Quoting [hidden email]:
>
>> [hidden email]:
>>>
>>> Quoting [hidden email]:
>>>
>>> > Wietse Venema:
>>> >> Joel Linn:
>>> >> > Why is it chosen to "not support stored procedures" instead of
>>> adding two
>>> >> > lines of code?
>>> >>
>>> >> The original mysql client may well have been written at a time that
>>> >> stored procedures did not exist, or no API documentation existed
>>> >> for how to do this correctly.
>>> >>
>>> >> If you feel strongly about stored procedures, then you are welcome
>>> >> to contribute code. If you can demonstrate that the code uses the
>>> >> MySQL API correctly (instead of "there are no error messages") then
>>> >> that would help getting the code accepted.
>>> >
>>> > This is a pointer to MySQL documentation for multi-statement support,
>>> > which seems to be needed to implement stored-procedure support in the
>>> > Postfix MySQL client.
>>> >
>>> > http://dev.mysql.com/doc/refman/5.7/en/c-api-multiple-queries.html
>>> >
>>> >     Wietse
>>>
>>> Thanks for the link.
>>> An interesting fact is that the doc says CLIENT_MULTI_RESULTS is
>>> default since MySQL 5.7.
>>> And comparing my error message "Commands out of sync" to the one
>>> in the discussion of 2008 John Fawcett dug out "can't return a
>>> result set in the given context", we are in fact not using the api
>>> as designed at the moment.  Sure its "just" an error message for
>>> both cases but the api thinks we can handle multiple results but
>>> in fact we can not.  Talking about multi statement support is
>>> planning one step ahead already, but a smart thought whatsoever,
>>> eliminating the need for stored procedures to some extend.
>>>
>>> I think we could at least support multi results by enumerating
>>> over all but the first result (return that) and maybe throw them
>>> away, at least issue a warning if they are additional result sets
>>> (not for statuses of course, they are OK if they indicate success).
>>> This way we can indicate to the administrator a poorly designed
>>> query that leaks data.  Maybe even issue an error for security
>>> reasons if the results contain more than one result set.  Because
>>> that could lead to undefined behavior if some SELECT prior to the
>>> correct SELECT accidentally gives wrong data (e.g. sending mail
>>> to mars).
>>>
>>> Multiple statements is another discussion, however subordinate and
>>> dependent on the implementation of multi results.  As far is I can
>>> see no prepared statements are used in the code so allowing for
>>> multiple statements is easy and shouldn't tension the security
>>> discussion (?), however sql injection is easier to do if you can
>>> end a statement with ";".  But I presume the % placeholders are
>>> filtered anyway (either explicit or implicit) so that shouldn't
>>> be a concern.
>>>
>>> Do you agree or do you see any drawbacks or problems?
>>
>> Unfortunately, I am not familiar with MySQL APIs, and I don't have
>> cycles to spare.
>>
>> My first priority is to ensure that existing Postfix code keeps
>> working as promised (i.e. Postfix does not break due to MySQL API
>> library changes).
>>
>> If the MySQL library will send multiple results even if Postfix
>> does not specify CLIENT_MULTI_RESULTS, it is sufficient to report
>> an error when the library returns more replies than expected?
>>
>> I'm not opposed to stored-procedure support, but I think it should
>> be implemented+documented by someone who understands the APIs, so
>> that would not be me.
>>
>>     Wietse
>
> At the moment no flags (0) are passed to mysql_real_connect.
> But if using the mysql api version 5.7 or newer the api "forces"
> CLIENT_MULTI_RESULTS.
> I see no way in the api doc to disable it, e.g. using
> mysql_set_server_option.
> So the code should no matter what cycle over the results anyway,
> because if not cleared they cause later queries to fail and
> we have no way to prohibit multiple results using the api afaIk.
I don't see how to disable CLIENT_MULTI_RESULTS either. However I
believe this is not a problem if you use a single query or are not using
stored procedures.
>
> So yes, looping results and generating an error for more than one
> result is sufficient to prevent hiccups in following queries.
>
Is this necessary? An error should already be generated if you try to
use stored procedures or multiple queries.
> That bugfix however would be very close to my suggested change.
> Is there someone maintaining the mysql portion of postfix,
> I mean except you keeping everything running?
>
> Joel
>
I can propose a code submission to add stored procedure support (based
on the proof of concept code from 2008), but the biggest part will be
doing the testing and non regression testing not the actual coding.

I believe the best approach to adding stored procedure support is to
iterate over multiple result sets and if there are exactly two and the
last one is the status result of the stored procedure and it is
successful then allow the lookup against the first result set. In all
other cases there should be an error for multiple result sets. In the
context of Postfix don't think it is useful to have more than one result
set generated by multiple queries or more than two result sets
(including the status one) from a stored procedure.

John

jl
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

jl

Quoting John Fawcett <[hidden email]>:

> I can propose a code submission to add stored procedure support (based
> on the proof of concept code from 2008), but the biggest part will be
> doing the testing and non regression testing not the actual coding.
>
> I believe the best approach to adding stored procedure support is to
> iterate over multiple result sets and if there are exactly two and the
> last one is the status result of the stored procedure and it is
> successful then allow the lookup against the first result set. In all
> other cases there should be an error for multiple result sets. In the
> context of Postfix don't think it is useful to have more than one result
> set generated by multiple queries or more than two result sets
> (including the status one) from a stored procedure.
>
> John

That is exactly what I had on my mind.

The specific error message would enable admins to exactly see where  
the trouble originates.
Also consuming all results still allows subsequent lookups to succeed  
normally.

I would of course volunteer to "test" the change per se but I'm afraid  
my knowledge of postfix's inner workings converges to zero, rendering  
my testing unrepresentative at last :/
Although the subset of postfix interfacing with mysql is quite small.

Joel

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

John Fawcett
On 07/04/2016 09:58 PM, [hidden email] wrote:

>
> Quoting John Fawcett <[hidden email]>:
>> I can propose a code submission to add stored procedure support (based
>> on the proof of concept code from 2008), but the biggest part will be
>> doing the testing and non regression testing not the actual coding.
>>
>> I believe the best approach to adding stored procedure support is to
>> iterate over multiple result sets and if there are exactly two and the
>> last one is the status result of the stored procedure and it is
>> successful then allow the lookup against the first result set. In all
>> other cases there should be an error for multiple result sets. In the
>> context of Postfix don't think it is useful to have more than one result
>> set generated by multiple queries or more than two result sets
>> (including the status one) from a stored procedure.
>>
>> John
>
> That is exactly what I had on my mind.
>
> The specific error message would enable admins to exactly see where
> the trouble originates.
> Also consuming all results still allows subsequent lookups to succeed
> normally.
>
> I would of course volunteer to "test" the change per se but I'm afraid
> my knowledge of postfix's inner workings converges to zero, rendering
> my testing unrepresentative at last :/
> Although the subset of postfix interfacing with mysql is quite small.
>
> Joel
>
Hi

here is my proposed submission to add mysql stored procedure support to
Postfix. As per Wietse's comments in the following thread

http://postfix.1071664.n5.nabble.com/fatal-table-lookup-problem-with-Postfix-lookup-call-to-MySQL-StoredProcedure-td13240.html

"I would be grateful if that support came with test cases for single
and multi-result queries, including non-error and error results so
that we can run the test whenever something in the code is changed."

One further thing to point out and that may need to be adjusted still:

An error is returned whenever:

- the check for further result sets gives an error instead of yes (0) or
no (-1)

- the stored procedure final result set with the status is not as
expected (mysql_field_count returns 0)

- there is more than one result set containing data.

While testing with postmap and multiple result sets I was able to
generate the following error message from postmap.c

"postmap: fatal: table mysql:./mysql-test2.cf: query error: Success"

The final "Success" comes from %m parameter reporting the errno. Since
no system call error has been returned, but it is an application error I
have forced errno to ENOTSUP. If instead the "Success" status is deemed
correct, then the statement setting errno and the inclusion of errno.h
can be elimianted without changing the basic functioning of the patch.

I am available to make any further adjustments needed. I look forward to
feedback from Joel or other interested people about more extensive testing.

John




patch.txt (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

Wietse Venema
John Fawcett:
> here is my proposed submission to add mysql stored procedure support to
> Postfix. As per Wietse's comments in the following thread

Thanks much. I'll examine it in the crumbs of available time.

        Wietse
jl
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

jl
In reply to this post by John Fawcett
Hey Guys,

this issue has decayed a bit but I now finally found the time (and the
nerves) to integrate the fix in my system.
I'm running Ubuntu 16.04 and trying not change to many things and be
able to have clean comparison I applied the patch to the apt sources and
only replaced the postfix-mysql package (ubuntu is using 3.1.0 it seems)
with my own.
I introduced some stored procedures and from my tests I conclude it
works. I will see in the next couple of days if there are issues I
overlooked.
I ran into an issue where I was returning no result set using
"smtpd_recipient_restrictions = check_recipient_access mysql:/[...]".
I'm not sure if that's an dict_mysql issue or caused by design upstream.
I overcame that by doing "select 1 from dual where false" when I don't
have anything else to return, which basically is an empty result set.

Thanks very much for your work,
Joel

On 2016-07-07 00:46, John Fawcett wrote:

> On 07/04/2016 09:58 PM, [hidden email] wrote:
>>
>> Quoting John Fawcett <[hidden email]>:
>>> I can propose a code submission to add stored procedure support
>>> (based
>>> on the proof of concept code from 2008), but the biggest part will be
>>> doing the testing and non regression testing not the actual coding.
>>>
>>> I believe the best approach to adding stored procedure support is to
>>> iterate over multiple result sets and if there are exactly two and
>>> the
>>> last one is the status result of the stored procedure and it is
>>> successful then allow the lookup against the first result set. In all
>>> other cases there should be an error for multiple result sets. In the
>>> context of Postfix don't think it is useful to have more than one
>>> result
>>> set generated by multiple queries or more than two result sets
>>> (including the status one) from a stored procedure.
>>>
>>> John
>>
>> That is exactly what I had on my mind.
>>
>> The specific error message would enable admins to exactly see where
>> the trouble originates.
>> Also consuming all results still allows subsequent lookups to succeed
>> normally.
>>
>> I would of course volunteer to "test" the change per se but I'm afraid
>> my knowledge of postfix's inner workings converges to zero, rendering
>> my testing unrepresentative at last :/
>> Although the subset of postfix interfacing with mysql is quite small.
>>
>> Joel
>>
> Hi
>
> here is my proposed submission to add mysql stored procedure support to
> Postfix. As per Wietse's comments in the following thread
>
> http://postfix.1071664.n5.nabble.com/fatal-table-lookup-problem-with-Postfix-lookup-call-to-MySQL-StoredProcedure-td13240.html
>
> "I would be grateful if that support came with test cases for single
> and multi-result queries, including non-error and error results so
> that we can run the test whenever something in the code is changed."
>
> One further thing to point out and that may need to be adjusted still:
>
> An error is returned whenever:
>
> - the check for further result sets gives an error instead of yes (0)
> or
> no (-1)
>
> - the stored procedure final result set with the status is not as
> expected (mysql_field_count returns 0)
>
> - there is more than one result set containing data.
>
> While testing with postmap and multiple result sets I was able to
> generate the following error message from postmap.c
>
> "postmap: fatal: table mysql:./mysql-test2.cf: query error: Success"
>
> The final "Success" comes from %m parameter reporting the errno. Since
> no system call error has been returned, but it is an application error
> I
> have forced errno to ENOTSUP. If instead the "Success" status is deemed
> correct, then the statement setting errno and the inclusion of errno.h
> can be elimianted without changing the basic functioning of the patch.
>
> I am available to make any further adjustments needed. I look forward
> to
> feedback from Joel or other interested people about more extensive
> testing.
>
> John

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

John Fawcett
On 11/22/2016 01:35 AM, Joel Linn wrote:

> Hey Guys,
>
> this issue has decayed a bit but I now finally found the time (and the
> nerves) to integrate the fix in my system.
> I'm running Ubuntu 16.04 and trying not change to many things and be
> able to have clean comparison I applied the patch to the apt sources
> and only replaced the postfix-mysql package (ubuntu is using 3.1.0 it
> seems) with my own.
> I introduced some stored procedures and from my tests I conclude it
> works. I will see in the next couple of days if there are issues I
> overlooked.
> I ran into an issue where I was returning no result set using
> "smtpd_recipient_restrictions = check_recipient_access mysql:/[...]".
> I'm not sure if that's an dict_mysql issue or caused by design
> upstream. I overcame that by doing "select 1 from dual where false"
> when I don't have anything else to return, which basically is an empty
> result set.
>
> Thanks very much for your work,
> Joel
Joel

can you provide some more information to reproduce the issue you mentioned?

John
jl
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

jl
On 2016-11-23 21:57, John Fawcett wrote:

> On 11/22/2016 01:35 AM, Joel Linn wrote:
>> Hey Guys,
>>
>> this issue has decayed a bit but I now finally found the time (and the
>> nerves) to integrate the fix in my system.
>> I'm running Ubuntu 16.04 and trying not change to many things and be
>> able to have clean comparison I applied the patch to the apt sources
>> and only replaced the postfix-mysql package (ubuntu is using 3.1.0 it
>> seems) with my own.
>> I introduced some stored procedures and from my tests I conclude it
>> works. I will see in the next couple of days if there are issues I
>> overlooked.
>> I ran into an issue where I was returning no result set using
>> "smtpd_recipient_restrictions = check_recipient_access mysql:/[...]".
>> I'm not sure if that's an dict_mysql issue or caused by design
>> upstream. I overcame that by doing "select 1 from dual where false"
>> when I don't have anything else to return, which basically is an empty
>> result set.
>>
>> Thanks very much for your work,
>> Joel
> Joel
>
> can you provide some more information to reproduce the issue you
> mentioned?
>
> John
Sure I can. Have a look at my log:

Nov 23 22:42:49 leuchtkanone postfix/smtpd[5863]: connect from
divmail1.helmholtz-berlin.de[134.30.104.21]
Nov 23 22:42:49 leuchtkanone postfix/smtpd[5863]: warning:
mysql:/etc/postfix/sql/recipient-access.cf: table lookup problem
Nov 23 22:42:49 leuchtkanone postfix/smtpd[5863]: NOQUEUE: reject: RCPT
from divmail1.helmholtz-berlin.de[134.30.104.21]: 451 4.3.5
<[hidden email]>: Recipient address rejected: Server configuration
error; from=<[hidden email]> to=<[hidden email]>
proto=ESMTP helo=<divmail1.helmholtz-berlin.de>

I used this empty test procedure (query = CALL ret_empty;):

CREATE DEFINER=`vmail`@`localhost` PROCEDURE `ret_empty`()
BEGIN
END

In my procedure I had some code paths return a result and some not. Like
I said i'm now using "SELECT 1 FROM dual WHERE false;" for those dead
paths, this simulates the behavior of the previous solution (one line
query) which always returns a result set, even if it's empty.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

John Fawcett
On 11/23/2016 10:54 PM, [hidden email] wrote:

> On 2016-11-23 21:57, John Fawcett wrote:
>> On 11/22/2016 01:35 AM, Joel Linn wrote:
>>> Hey Guys,
>>>
>>> this issue has decayed a bit but I now finally found the time (and the
>>> nerves) to integrate the fix in my system.
>>> I'm running Ubuntu 16.04 and trying not change to many things and be
>>> able to have clean comparison I applied the patch to the apt sources
>>> and only replaced the postfix-mysql package (ubuntu is using 3.1.0 it
>>> seems) with my own.
>>> I introduced some stored procedures and from my tests I conclude it
>>> works. I will see in the next couple of days if there are issues I
>>> overlooked.
>>> I ran into an issue where I was returning no result set using
>>> "smtpd_recipient_restrictions = check_recipient_access mysql:/[...]".
>>> I'm not sure if that's an dict_mysql issue or caused by design
>>> upstream. I overcame that by doing "select 1 from dual where false"
>>> when I don't have anything else to return, which basically is an empty
>>> result set.
>>>
>>> Thanks very much for your work,
>>> Joel
>> Joel
>>
>> can you provide some more information to reproduce the issue you
>> mentioned?
>>
>> John
> Sure I can. Have a look at my log:
>
> Nov 23 22:42:49 leuchtkanone postfix/smtpd[5863]: connect from
> divmail1.helmholtz-berlin.de[134.30.104.21]
> Nov 23 22:42:49 leuchtkanone postfix/smtpd[5863]: warning:
> mysql:/etc/postfix/sql/recipient-access.cf: table lookup problem
> Nov 23 22:42:49 leuchtkanone postfix/smtpd[5863]: NOQUEUE: reject:
> RCPT from divmail1.helmholtz-berlin.de[134.30.104.21]: 451 4.3.5
> <[hidden email]>: Recipient address rejected: Server configuration
> error; from=<[hidden email]> to=<[hidden email]>
> proto=ESMTP helo=<divmail1.helmholtz-berlin.de>
>
> I used this empty test procedure (query = CALL ret_empty;):
>
> CREATE DEFINER=`vmail`@`localhost` PROCEDURE `ret_empty`()
> BEGIN
> END
>
> In my procedure I had some code paths return a result and some not.
> Like I said i'm now using "SELECT 1 FROM dual WHERE false;" for those
> dead paths, this simulates the behavior of the previous solution (one
> line query) which always returns a result set, even if it's empty.

Joel

with MySQL procedures every SELECT statement that is executed produces a
result set. In my patch I explicitly check to make sure there are no
multiple result sets, but I do not check for no result. That could be
improved upon, to give a specific warning that no result sets were
returned. However, that will always be a lookup failure. The stored
procedure (based on the way the patch is written) must always return a
result set even if that result set is empty.

Your solution with returning an empty result set when there is no other
result (SELECT 1 FROM DUAL WHERE FALSE) looks correct.

John

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

John Fawcett
On 11/27/2016 01:47 PM, John Fawcett wrote:

> On 11/23/2016 10:54 PM, [hidden email] wrote:
>> On 2016-11-23 21:57, John Fawcett wrote:
>>> On 11/22/2016 01:35 AM, Joel Linn wrote:
>>>> Hey Guys,
>>>>
>>>> this issue has decayed a bit but I now finally found the time (and the
>>>> nerves) to integrate the fix in my system.
>>>> I'm running Ubuntu 16.04 and trying not change to many things and be
>>>> able to have clean comparison I applied the patch to the apt sources
>>>> and only replaced the postfix-mysql package (ubuntu is using 3.1.0 it
>>>> seems) with my own.
>>>> I introduced some stored procedures and from my tests I conclude it
>>>> works. I will see in the next couple of days if there are issues I
>>>> overlooked.
>>>> I ran into an issue where I was returning no result set using
>>>> "smtpd_recipient_restrictions = check_recipient_access mysql:/[...]".
>>>> I'm not sure if that's an dict_mysql issue or caused by design
>>>> upstream. I overcame that by doing "select 1 from dual where false"
>>>> when I don't have anything else to return, which basically is an empty
>>>> result set.
>>>>
>>>> Thanks very much for your work,
>>>> Joel
>>> Joel
>>>
>>> can you provide some more information to reproduce the issue you
>>> mentioned?
>>>
>>> John
>> Sure I can. Have a look at my log:
>>
>> Nov 23 22:42:49 leuchtkanone postfix/smtpd[5863]: connect from
>> divmail1.helmholtz-berlin.de[134.30.104.21]
>> Nov 23 22:42:49 leuchtkanone postfix/smtpd[5863]: warning:
>> mysql:/etc/postfix/sql/recipient-access.cf: table lookup problem
>> Nov 23 22:42:49 leuchtkanone postfix/smtpd[5863]: NOQUEUE: reject:
>> RCPT from divmail1.helmholtz-berlin.de[134.30.104.21]: 451 4.3.5
>> <[hidden email]>: Recipient address rejected: Server configuration
>> error; from=<[hidden email]> to=<[hidden email]>
>> proto=ESMTP helo=<divmail1.helmholtz-berlin.de>
>>
>> I used this empty test procedure (query = CALL ret_empty;):
>>
>> CREATE DEFINER=`vmail`@`localhost` PROCEDURE `ret_empty`()
>> BEGIN
>> END
>>
>> In my procedure I had some code paths return a result and some not.
>> Like I said i'm now using "SELECT 1 FROM dual WHERE false;" for those
>> dead paths, this simulates the behavior of the previous solution (one
>> line query) which always returns a result set, even if it's empty.
> Joel
>
> with MySQL procedures every SELECT statement that is executed produces a
> result set. In my patch I explicitly check to make sure there are no
> multiple result sets, but I do not check for no result. That could be
> improved upon, to give a specific warning that no result sets were
> returned. However, that will always be a lookup failure. The stored
> procedure (based on the way the patch is written) must always return a
> result set even if that result set is empty.
>
> Your solution with returning an empty result set when there is no other
> result (SELECT 1 FROM DUAL WHERE FALSE) looks correct.
>
> John
>
Revised patch to improve error reporting when no result set containing
data is returned

diff -ur postfix-3.2-20161106-orig/proto/mysql_table
postfix-3.2-20161106/proto/mysql_table
--- postfix-3.2-20161106-orig/proto/mysql_table 2016-10-01
15:44:52.000000000 +0200
+++ postfix-3.2-20161106/proto/mysql_table      2016-11-27
10:08:05.535236131 +0100
@@ -289,6 +289,39 @@
 #      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. Multiple result sets are not supported and will
+#      produce a warning and no valid result. 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
+#      data with a single result set:
+#
+# .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-20161106-orig/src/global/dict_mysql.c
postfix-3.2-20161106/src/global/dict_mysql.c
--- postfix-3.2-20161106-orig/src/global/dict_mysql.c   2016-09-25
17:27:11.000000000 +0200
+++ postfix-3.2-20161106/src/global/dict_mysql.c        2016-11-27
13:56:50.956334035 +0100
@@ -187,6 +187,7 @@
 #include <time.h>
 #include <mysql.h>
 #include <limits.h>
+#include <errno.h>

 #ifdef STRCASECMP_IN_STRINGS_H
 #include <strings.h>
@@ -519,6 +520,10 @@
 {
     HOST   *host;
     MYSQL_RES *res = 0;
+    MYSQL_RES *temp_res = 0;
+    int status=0;
+    int num_rs=0;
+    int sp_error=0;

     while ((host = dict_mysql_get_active(dict_mysql)) != NULL) {
 #if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
@@ -536,9 +541,46 @@
 #endif

        if (!(mysql_query(host->db, vstring_str(query)))) {
-           if ((res = mysql_store_result(host->db)) == 0) {
-               msg_warn("mysql query failed: %s", mysql_error(host->db));
+           sp_error = 0;
+           num_rs = 0;
+           res = 0;
+           do {
+               temp_res = mysql_store_result(host->db);
+               /* did statement return data? */
+               if (temp_res) {
+                   num_rs++;
+                   if (num_rs > 1) {
+                       msg_warn("dict_mysql: multiple result sets
returning data are not supported");
+                       mysql_free_result(temp_res);
+                       temp_res = 0;
+                   } else {
+                       res = temp_res;
+                   }
+               } else {
+               /* no data, check if there were errors */
+                   if (mysql_field_count(host->db) == 0) {
+                       if (num_rs == 0) {
+                           msg_warn("dict_mysql: stored procedure did
not return any result set containing data");
+                       } else {
+                           if (msg_verbose)
+                               msg_info("dict_mysql: successful final
result for stored procedure");
+                       }
+                   } else {
+                       sp_error=1;
+                       msg_warn("dict_mysql: unsuccessful final result
for stored procedure: %s", mysql_error(host->db));
+                   }
+               }
+               /* more results? -1 = no, >0 = error, 0 = yes (keep
looping) */
+               if ((status = mysql_next_result(host->db)) > 0)
+                    msg_warn("mysql query failed (mysql_next_result):
%s", mysql_error(host->db));
+           } while (status == 0);
+           if (status != -1 || sp_error || num_rs != 1) {
                plmysql_down_host(host);
+               errno = ENOTSUP;
+               if (res) {
+                   mysql_free_result(res);
+                   res = 0;
+               }
            } else {
                if (msg_verbose)
                    msg_info("dict_mysql: successful query from host
%s", host->hostname);
@@ -587,7 +629,7 @@
                           dict_mysql->dbname,
                           host->port,
                           (host->type == TYPEUNIX ? host->name : 0),
-                          0)) {
+                          CLIENT_MULTI_RESULTS)) {
        if (msg_verbose)
            msg_info("dict_mysql: successful connection to host %s",
                     host->hostname);

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

Wietse Venema
I have been working this code into Postfix, and have a comment
about error reporting for old-style queries.

>      while ((host = dict_mysql_get_active(dict_mysql)) != NULL) {
>  #if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
> @@ -536,9 +541,46 @@
>  #endif
>
>         if (!(mysql_query(host->db, vstring_str(query)))) {
> -           if ((res = mysql_store_result(host->db)) == 0) {
> -               msg_warn("mysql query failed: %s", mysql_error(host->db));
> +           sp_error = 0;

Here, the old code reported "mysql query failed" when an old-style
query returned no result set.

> +           num_rs = 0;
> +           res = 0;
> +           do {
> +               temp_res = mysql_store_result(host->db);
> +               /* did statement return data? */
> +               if (temp_res) {
> +                   num_rs++;
> +                   if (num_rs > 1) {
> +                       msg_warn("dict_mysql: multiple result sets
> returning data are not supported");
> +                       mysql_free_result(temp_res);
> +                       temp_res = 0;
> +                   } else {
> +                       res = temp_res;
> +                   }
> +               } else {
> +               /* no data, check if there were errors */
> +                   if (mysql_field_count(host->db) == 0) {
> +                       if (num_rs == 0) {
> +                           msg_warn("dict_mysql: stored procedure did
> not return any result set containing data");
> +                       } else {
> +                           if (msg_verbose)
> +                               msg_info("dict_mysql: successful final
> result for stored procedure");
> +                       }
> +                   } else {
> +                       sp_error=1;
> +                       msg_warn("dict_mysql: unsuccessful final result
> for stored procedure: %s", mysql_error(host->db));

What if Postfix made an old-style query?  I think it should just
report the old-style error here.

        Wietse
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: AW: Possible Bug ? postfix 3.1.0-3 fails on mysql table lookup

John Fawcett
On 12/18/2016 02:09 AM, Wietse Venema wrote:

> I have been working this code into Postfix, and have a comment
> about error reporting for old-style queries.
>
>>      while ((host = dict_mysql_get_active(dict_mysql)) != NULL) {
>>  #if defined(MYSQL_VERSION_ID) && MYSQL_VERSION_ID >= 40000
>> @@ -536,9 +541,46 @@
>>  #endif
>>
>>         if (!(mysql_query(host->db, vstring_str(query)))) {
>> -           if ((res = mysql_store_result(host->db)) == 0) {
>> -               msg_warn("mysql query failed: %s", mysql_error(host->db));
>> +           sp_error = 0;
> Here, the old code reported "mysql query failed" when an old-style
> query returned no result set.
>
>> +           num_rs = 0;
>> +           res = 0;
>> +           do {
>> +               temp_res = mysql_store_result(host->db);
>> +               /* did statement return data? */
>> +               if (temp_res) {
>> +                   num_rs++;
>> +                   if (num_rs > 1) {
>> +                       msg_warn("dict_mysql: multiple result sets
>> returning data are not supported");
>> +                       mysql_free_result(temp_res);
>> +                       temp_res = 0;
>> +                   } else {
>> +                       res = temp_res;
>> +                   }
>> +               } else {
>> +               /* no data, check if there were errors */
>> +                   if (mysql_field_count(host->db) == 0) {
>> +                       if (num_rs == 0) {
>> +                           msg_warn("dict_mysql: stored procedure did
>> not return any result set containing data");
>> +                       } else {
>> +                           if (msg_verbose)
>> +                               msg_info("dict_mysql: successful final
>> result for stored procedure");
>> +                       }
>> +                   } else {
>> +                       sp_error=1;
>> +                       msg_warn("dict_mysql: unsuccessful final result
>> for stored procedure: %s", mysql_error(host->db));
> 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

12
Loading...