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

[Joel Linn]

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: owner-postfix-***@postfix.org
[mailto:owner-postfix-***@postfix.org] Im Auftrag von John Fawcett
Gesendet: Sonntag, 3. Juli 2016 18:10
An: postfix-***@postfix.org
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

[Wietse Venema]

Post by 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

[Wietse Venema]

Post by Wietse Venema

Post by 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

[j***@conductive.de]

Post by Wietse Venema

Post by Wietse Venema

Post by 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

[Wietse Venema]

Post by j***@conductive.de

Post by Wietse Venema

Post by Wietse Venema

Post by 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

[j***@conductive.de]

Post by Wietse Venema

Post by Joel Linn

Post by Wietse Venema

Post by Wietse Venema

Post by Joel Linn
Why is it chosen to "not support stored procedures" instead of

adding two

Post by Wietse Venema

Post by Wietse Venema

Post by Joel Linn
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

[Wietse Venema]

Post by j***@conductive.de
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

[John Fawcett]

Post by j***@conductive.de

Post by Wietse Venema

Post by Joel Linn

Post by Wietse Venema

Post by Wietse Venema

Post by Joel Linn
Why is it chosen to "not support stored procedures" instead of

adding two

Post by Wietse Venema

Post by Wietse Venema

Post by Joel Linn
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.

Post by j***@conductive.de
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.

Post by j***@conductive.de
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

[j***@conductive.de]

Post by John Fawcett
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

[John Fawcett]

Post by j***@conductive.de

Post by John Fawcett
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

[Wietse Venema]

Post by 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