Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Michael Paquier-2
Hi all,

As $subject says, pg_test_fsync and pg_test_timing don't really check
the range of option values specified.  It is possible for example to
make pg_test_fsync run an infinite amount of time, and pg_test_timing
does not handle overflows with --duration at all.

These are far from being critical issues, but let's fix them at least
on HEAD.  So, please see the attached, where I have also added some
basic TAP tests for both tools.

Thanks,
--
Michael

pgtest-fix-range.patch (5K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Peter Eisentraut-6
On 2020-08-06 08:27, Michael Paquier wrote:
> As $subject says, pg_test_fsync and pg_test_timing don't really check
> the range of option values specified.  It is possible for example to
> make pg_test_fsync run an infinite amount of time, and pg_test_timing
> does not handle overflows with --duration at all.
>
> These are far from being critical issues, but let's fix them at least
> on HEAD.  So, please see the attached, where I have also added some
> basic TAP tests for both tools.

According to the POSIX standard, atoi() is not required to do any error
checking, and if you want error checking, you should use strtol().

And if you do that, you might as well change the variables to unsigned
and use strtoul(), and then drop the checks for <=0.  I would allow 0.
It's not very useful, but it's not harmful and could be applicable in
testing.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Michael Paquier-2
On Fri, Sep 04, 2020 at 11:24:39PM +0200, Peter Eisentraut wrote:
> According to the POSIX standard, atoi() is not required to do any error
> checking, and if you want error checking, you should use strtol().
>
> And if you do that, you might as well change the variables to unsigned and
> use strtoul(), and then drop the checks for <=0.

Switching to unsigned makes sense, indeed.

> I would allow 0. It's not
> very useful, but it's not harmful and could be applicable in testing.

Hmm, OK.  For pg_test_fsync, 0 means infinity, and for pg_test_timing
that means stopping immediately (we currently don't allow that).  How
does this apply to testing?  For pg_test_fsync, using 0 would mean to
just remain stuck in the first fsync() pattern, while for
pg_test_fsync this means doing no test loops at all, generating a
useless log once done.  Or do you mean to change the logic of
pg_test_fsync so as --secs-per-test=0 means doing one single write?
That's something I thought about for this thread, but I am not sure
that the extra regression test gain is worth more complexity in this
code.
--
Michael

pgtest-fix-range-v2.patch (6K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Peter Eisentraut-6
On 2020-09-06 05:04, Michael Paquier wrote:

>> I would allow 0. It's not
>> very useful, but it's not harmful and could be applicable in testing.
>
> Hmm, OK.  For pg_test_fsync, 0 means infinity, and for pg_test_timing
> that means stopping immediately (we currently don't allow that).  How
> does this apply to testing?  For pg_test_fsync, using 0 would mean to
> just remain stuck in the first fsync() pattern, while for
> pg_test_fsync this means doing no test loops at all, generating a
> useless log once done.  Or do you mean to change the logic of
> pg_test_fsync so as --secs-per-test=0 means doing one single write?
> That's something I thought about for this thread, but I am not sure
> that the extra regression test gain is worth more complexity in this
> code.

I think in general doing something 0 times should be allowed if possible.

However, I see that in the case of pg_test_fsync you end up in alarm(0),
which does something different, so it's okay in that case to disallow it.

I notice that the error checking you introduce is different from the
checks for pgbench -t and -T (the latter having no errno checks).  I'm
not sure which is correct, but it's perhaps worth making them the same.

(pgbench -t 0, which is also currently not allowed, is a good example of
why this could be useful, because that would allow checking whether the
script etc. can be loaded without running an actual test.)

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Michael Paquier-2
On Mon, Sep 07, 2020 at 10:06:57AM +0200, Peter Eisentraut wrote:
> However, I see that in the case of pg_test_fsync you end up in alarm(0),
> which does something different, so it's okay in that case to disallow it.

Yep.

> I notice that the error checking you introduce is different from the checks
> for pgbench -t and -T (the latter having no errno checks).  I'm not sure
> which is correct, but it's perhaps worth making them the same.

pgbench currently uses atoi() to parse the options of -t and -T.  Are
you suggesting to switch that to strtoXX() as well or perhaps you are
referring to the parsing of the weight in parseScriptWeight()?  FWIW,
the error handling introduced in this patch is similar to what we do
for example in pg_resetwal.  This has its own problems as strtoul()
would not report ERANGE except for values higher than ULONG_MAX, but
the returned results are stored in 32 bits.  We could switch to just
use uint64 where we could of course, but is that really worth it for
such tools?  For example, pg_test_timing could overflow the
total_timing calculated if using a too high value, but nobody would
use such values anyway.  So I'd rather just use uint32 and call it a
day, for simplicity's sake mainly..

> (pgbench -t 0, which is also currently not allowed, is a good example of why
> this could be useful, because that would allow checking whether the script
> etc. can be loaded without running an actual test.)

Perhaps.  That looks like a separate item to me though.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Peter Eisentraut-6
On 2020-09-10 09:59, Michael Paquier wrote:

>> I notice that the error checking you introduce is different from the checks
>> for pgbench -t and -T (the latter having no errno checks).  I'm not sure
>> which is correct, but it's perhaps worth making them the same.
> pgbench currently uses atoi() to parse the options of -t and -T.  Are
> you suggesting to switch that to strtoXX() as well or perhaps you are
> referring to the parsing of the weight in parseScriptWeight()?  FWIW,
> the error handling introduced in this patch is similar to what we do
> for example in pg_resetwal.  This has its own problems as strtoul()
> would not report ERANGE except for values higher than ULONG_MAX, but
> the returned results are stored in 32 bits.  We could switch to just
> use uint64 where we could of course, but is that really worth it for
> such tools?  For example, pg_test_timing could overflow the
> total_timing calculated if using a too high value, but nobody would
> use such values anyway.  So I'd rather just use uint32 and call it a
> day, for simplicity's sake mainly..

The first patch you proposed checks for errno == ERANGE, but pgbench
code doesn't do that.  So one of them is not correct.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Michael Paquier-2
On Thu, Sep 10, 2020 at 03:59:20PM +0200, Peter Eisentraut wrote:
> The first patch you proposed checks for errno == ERANGE, but pgbench code
> doesn't do that.  So one of them is not correct.

Sorry for the confusion, I misunderstood what you were referring to.
Yes, the first patch is wrong to add the check on errno.  FWIW, I
thought about your point to use strtol() but that does not seem worth
the complication for those tools.  It is not like anybody is going to
use high values for these, and using uint64 to make sure that the
boundaries are checked just add more checks for bounds.  There is
one example in pg_test_timing when compiling the total time.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Peter Eisentraut-6
On 2020-09-11 09:08, Michael Paquier wrote:

> On Thu, Sep 10, 2020 at 03:59:20PM +0200, Peter Eisentraut wrote:
>> The first patch you proposed checks for errno == ERANGE, but pgbench code
>> doesn't do that.  So one of them is not correct.
>
> Sorry for the confusion, I misunderstood what you were referring to.
> Yes, the first patch is wrong to add the check on errno.  FWIW, I
> thought about your point to use strtol() but that does not seem worth
> the complication for those tools.  It is not like anybody is going to
> use high values for these, and using uint64 to make sure that the
> boundaries are checked just add more checks for bounds.  There is
> one example in pg_test_timing when compiling the total time.

I didn't mean use strtol() to be able to process larger values, but for
the error checking.  atoi() cannot detect any errors other than ERANGE.
So if you are spending effort on making the option value parsing more
robust, relying on atoi() will result in an incomplete solution.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Range checks of pg_test_fsync --secs-per-test and pg_test_timing --duration

Michael Paquier-2
On Tue, Sep 15, 2020 at 02:39:08PM +0200, Peter Eisentraut wrote:
> I didn't mean use strtol() to be able to process larger values, but for the
> error checking.  atoi() cannot detect any errors other than ERANGE. So if
> you are spending effort on making the option value parsing more robust,
> relying on atoi() will result in an incomplete solution.

Okay, after looking at that, here is v3.  This includes range checks
as well as errno checks based on strtol().  What do you think?
--
Michael

pgtest-fix-range-v3.patch (7K) Download Attachment
signature.asc (849 bytes) Download Attachment