Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: prefer var = time(NULL) over time(&var) #13815

Closed
wants to merge 2 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented May 28, 2024

Following up on previous occurrences showing up as gcc warnings, replace
the remaining time(&var) calls with var = time(NULL), though these
aren't specifically causing compiler warnings. These are in the TFTP
client code (lib/tftp.c), except one which is in a debug branch in
lib/http_aws_sigv4.c.

What's unexplainable is that this patch seems to mitigate TFTP tests
often hanging or going into an infinite loop on GHA windows workflows
with MSYS2, mingw-w64 and MSVC (Cygwin is unaffected):
#13599 (comment)
TFTP hangs did not entirely disappear though, so could be unrelated.

time() docs:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/time-time32-time64
https://manpages.debian.org/bookworm/manpages-dev/time.2.en.html

Follow-up to 58ca0a2 #13800
Follow-up to d0728c9 #13643
Closes #13815


More test runs here: vszakats#1

Following up on previous occurrences showing up as gcc warnings, replace
the remaining few `time(&var)` calls with `= time(NULL)`, though these
aren't specifically causing compiler warnings. All of these are in the
TFTP client code (`lib/tftp.c`), except one which is in a debug branch
in `lib/http_aws_sigv4`.

What's unexplainable is that this patch seems to stabilize the TFTP
tests regularly hanging or going into an infinite loop on GHA windows
workflows with MSYS2, mingw-w64 and MSVC (Cygwin is unaffected):
  curl#13599 (comment)

I figure it's something worth trying even if it turns out to be a dead
end.

`time()` docs:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/time-time32-time64
https://manpages.debian.org/bookworm/manpages-dev/time.2.en.html

Follow-up to 58ca0a2 curl#13800
Follow-up to d0728c9 curl#13643
Closes #xxxxx
@github-actions github-actions bot added the CI Continuous Integration label May 28, 2024
@vszakats
Copy link
Member Author

vszakats commented May 28, 2024

Okay, so got hung again right away this time; hard to tell where:
https://github.com/curl/curl/actions/runs/9273526999/job/25513835652?pr=13815#step:6:4437
This seems independent of TFTP, it hung in a separate PR too:
https://github.com/curl/curl/actions/runs/9273775947/job/25514611274?pr=13817
What a mess.

@vszakats vszakats marked this pull request as draft May 28, 2024 17:45
@vszakats vszakats marked this pull request as ready for review May 28, 2024 18:05
@jay
Copy link
Member

jay commented May 28, 2024

That's truly weird. I wonder if this is a 32/64 bit issue, the msdn time doc says it's supposed to be 64 bit in most cases, and so maybe the reason for the weirdness is time(&foo) is either the function is expecting a 32-bit and the variable is 64 bit or the other way around, and it's writing outside its memory or half the memory isn't written?

try using a 64-bit variable explicitly, like
{
__time64_t now = 0xFFFFFFFFFFFFFFFFi64;
time(&now);
DEBUGASSERT(now > 0);
DEBUGASSERT((unsigned __int64)now < 0xFFFFFFFFUL);
DEBUGASSERT(sizeof(time_t) == 8);
}

edit: oops I forgot about the integer promotion (now < 0xFFFFFFFFUL); so I modified the assertion to cast to 64 bit unsigned

edit2: added another assert to see if it's Dan's issue

@dfandrich
Copy link
Contributor

Truly weird is also a good description of the possibly related TFTP timeout issues I found in #12040. The problem there appeared to be a simple time(&var) call was being set to 0.

@vszakats
Copy link
Member Author

I could not spot any issues with time_t size, or the wrong time() flavour being called. This seems like a lucky coincidence. Also TFTP is still able to hang with this patch in place:
https://github.com/vszakats/curl/actions/runs/9278913609/job/25530650928#step:14:13754

It'd probably still be a good PR to merge (without re-enabling TFTP tests) just to do these calls the same way across the codebase. The assigment style can also avoid the EOVERFLOW failure mode on Linux at least.

As a next step maybe it'd be useful to check for a -1 value returned, as Jay suggested.

@vszakats
Copy link
Member Author

vszakats commented May 29, 2024

Another idea is to replace remaining direct time() calls with Curl_now(), which is using more robust alternatives on popular platforms.

edit: modules using time(): altsvc, cookie, hostip, hsts, http_aws_sigv4, tftp, gtls.

@jay: Do you vote to re-enable TFTP tests with this patch, or leave them disabled? We can always disable them again in a separate PR – or try other fixes.

@jay
Copy link
Member

jay commented May 29, 2024

Actually I would leave them disabled because they don't seem to work properly for mingw AFAICS, and based on what you've said and no reports other than us I don't think it's a curl bug

@jay
Copy link
Member

jay commented May 29, 2024

Another idea is to replace remaining direct time() calls with Curl_now(), which is using more robust alternatives on popular platforms.

Curl_now and time are not synonymous, so that would have to be approached very carefully and I don't know if it's worth the work

@jay
Copy link
Member

jay commented May 29, 2024

Curl_now and time are not synonymous, so that would have to be approached very carefully and I don't know if it's worth the work

I reviewed the time calls in tftp and they are all based around recording the receive timestamp so they could easily be changed to Curl_now but I don't see how it would be better.

edit: Curl_now would be better because it seems for many operating systems the value only increments based on a fixed point in comparison to time which can return a lesser value if the time is changed to an earlier time (eg from Jun 5 11p to Jun 5 9p or something)

@dfandrich
Copy link
Contributor

dfandrich commented May 29, 2024 via email

@vszakats
Copy link
Member Author

vszakats commented May 29, 2024

OK, let's leave TFTP tests disabled. It still leaves some hangs to deal with, but it removes TFTP as a culprit.

@vszakats vszakats closed this in 3b9569c May 29, 2024
@vszakats vszakats deleted the time-tidy branch May 29, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration tests TFTP
Development

Successfully merging this pull request may close these issues.

None yet

3 participants