From f8f040e6596fa22b68198adf42dc6adcedfa57f0 Mon Sep 17 00:00:00 2001 From: Ryan Winograd Date: Mon, 26 Jun 2017 11:51:05 -0500 Subject: [PATCH] progress: prevent resetting t_starttransfer Prevent `Curl_pgrsTime` from modifying `t_starttransfer` when invoked with `TIMER_STARTTRANSFER` more than once during a single request. When a redirect occurs, this is considered a new request and `t_starttransfer` can be updated to reflect the `t_starttransfer` time of the redirect request. Closes #1616 Bug: https://github.com/curl/curl/pull/1602#issuecomment-310267370 --- lib/progress.c | 16 ++++++- tests/data/Makefile.inc | 2 +- tests/data/test1399 | 26 +++++++++++ tests/unit/Makefile.inc | 4 ++ tests/unit/unit1399.c | 96 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 tests/data/test1399 create mode 100644 tests/unit/unit1399.c diff --git a/lib/progress.c b/lib/progress.c index 457b4bd03..e92d96fdc 100644 --- a/lib/progress.c +++ b/lib/progress.c @@ -161,6 +161,9 @@ void Curl_pgrsResetTimesSizes(struct Curl_easy *data) Curl_pgrsSetUploadSize(data, -1); } +/* + * @unittest: 1399 + */ void Curl_pgrsTime(struct Curl_easy *data, timerid timer) { struct timeval now = Curl_tvnow(); @@ -196,7 +199,18 @@ void Curl_pgrsTime(struct Curl_easy *data, timerid timer) break; case TIMER_STARTTRANSFER: delta = &data->progress.t_starttransfer; - break; + /* prevent updating t_starttransfer unless: + * 1) this is the first time we're setting t_starttransfer + * 2) a redirect has occurred since the last time t_starttransfer was set + * This prevents repeated invocations of the function from incorrectly + * changing the t_starttransfer time. + */ + if (*delta > data->progress.t_redirect) { + return; + } + else { + break; + } case TIMER_POSTRANSFER: /* this is the normal end-of-transfer thing */ break; diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc index 64eb0619b..3f64f86c9 100644 --- a/tests/data/Makefile.inc +++ b/tests/data/Makefile.inc @@ -146,7 +146,7 @@ test1364 test1365 test1366 test1367 test1368 test1369 test1370 test1371 \ test1372 test1373 test1374 test1375 test1376 test1377 test1378 test1379 \ test1380 test1381 test1382 test1383 test1384 test1385 test1386 test1387 \ test1388 test1389 test1390 test1391 test1392 test1393 test1394 test1395 \ -test1396 test1397 test1398 \ +test1396 test1397 test1398 test1399 \ \ test1400 test1401 test1402 test1403 test1404 test1405 test1406 test1407 \ test1408 test1409 test1410 test1411 test1412 test1413 test1414 test1415 \ diff --git a/tests/data/test1399 b/tests/data/test1399 new file mode 100644 index 000000000..fe3879df1 --- /dev/null +++ b/tests/data/test1399 @@ -0,0 +1,26 @@ + + + +unittest +Curl_pgrsTime + + + +# +# Client-side + + +none + + +unittest + + +Curl_pgrsTime unit tests + + +unit1399 + + + + diff --git a/tests/unit/Makefile.inc b/tests/unit/Makefile.inc index ee01627df..1320b4a67 100644 --- a/tests/unit/Makefile.inc +++ b/tests/unit/Makefile.inc @@ -7,6 +7,7 @@ UNITFILES = curlcheck.h \ # These are all unit test programs UNITPROGS = unit1300 unit1301 unit1302 unit1303 unit1304 unit1305 unit1307 \ unit1308 unit1309 unit1330 unit1394 unit1395 unit1396 unit1397 unit1398 \ + unit1399 \ unit1600 unit1601 unit1602 unit1603 unit1604 unit1605 unit1606 unit1300_SOURCES = unit1300.c $(UNITFILES) @@ -57,6 +58,9 @@ unit1397_CPPFLAGS = $(AM_CPPFLAGS) unit1398_SOURCES = unit1398.c $(UNITFILES) unit1398_CPPFLAGS = $(AM_CPPFLAGS) +unit1399_SOURCES = unit1399.c $(UNITFILES) +unit1399_CPPFLAGS = $(AM_CPPFLAGS) + unit1600_SOURCES = unit1600.c $(UNITFILES) unit1600_CPPFLAGS = $(AM_CPPFLAGS) diff --git a/tests/unit/unit1399.c b/tests/unit/unit1399.c new file mode 100644 index 000000000..1befc8aaf --- /dev/null +++ b/tests/unit/unit1399.c @@ -0,0 +1,96 @@ +/*************************************************************************** + * _ _ ____ _ + * Project ___| | | | _ \| | + * / __| | | | |_) | | + * | (__| |_| | _ <| |___ + * \___|\___/|_| \_\_____| + * + * Copyright (C) 1998 - 2017, Daniel Stenberg, , et al. + * + * This software is licensed as described in the file COPYING, which + * you should have received as part of this distribution. The terms + * are also available at https://curl.haxx.se/docs/copyright.html. + * + * You may opt to use, copy, modify, merge, publish, distribute and/or sell + * copies of the Software, and permit persons to whom the Software is + * furnished to do so, under the terms of the COPYING file. + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY + * KIND, either express or implied. + * + ***************************************************************************/ +#include "curlcheck.h" + +#include "urldata.h" +#include "progress.h" + +static int usec_magnitude = 1000000; + +static bool unit_setup(void) +{ + return CURLE_OK; +} + +static void unit_stop(void) +{ + +} + +static bool usec_matches_seconds(time_t time_usec, int expected_seconds) +{ + int time_sec = (int)(time_usec / usec_magnitude); + return time_sec == expected_seconds; +} + +UNITTEST_START + struct Curl_easy data; + struct timeval now = Curl_tvnow(); + + data.progress.t_starttransfer = 0; + data.progress.t_redirect = 0; + + /* + * Set the startsingle time to a second ago. This time is used by + * Curl_pgrsTime to calculate how much time the events takes. + * t_starttransfer should be updated to reflect the difference from this time + * when `Curl_pgrsTime is invoked. + */ + data.progress.t_startsingle.tv_sec = now.tv_sec - 1; + data.progress.t_startsingle.tv_usec = now.tv_usec; + + Curl_pgrsTime(&data, TIMER_STARTTRANSFER); + + fail_unless(usec_matches_seconds(data.progress.t_starttransfer, 1), + "about 1 second should have passed"); + + /* + * Update the startsingle time to a second ago to simulate another second has + * passed. + * Now t_starttransfer should not be changed, as t_starttransfer has already + * occurred and another invocation of `Curl_pgrsTime` for TIMER_STARTTRANSFER + * is superfluous. + */ + data.progress.t_startsingle.tv_sec = now.tv_sec - 2; + data.progress.t_startsingle.tv_usec = now.tv_usec; + + Curl_pgrsTime(&data, TIMER_STARTTRANSFER); + + fail_unless(usec_matches_seconds(data.progress.t_starttransfer, 1), + "about 1 second should have passed"); + + /* + * Simulate what happens after a redirect has occurred. + * + * Since the value of t_starttransfer is set to the value from the first + * request, it should be updated when a transfer occurs such that + * t_starttransfer is the starttransfer time of the redirect request. + */ + data.progress.t_startsingle.tv_sec = now.tv_sec - 3; + data.progress.t_startsingle.tv_usec = now.tv_usec; + data.progress.t_redirect = (now.tv_sec - 2) * usec_magnitude; + + Curl_pgrsTime(&data, TIMER_STARTTRANSFER); + + fail_unless(usec_matches_seconds(data.progress.t_starttransfer, 3), + "about 3 second should have passed"); +UNITTEST_STOP