Page MenuHomeWildfire Games

Fix undefined behavior and extend UserReporter cURL error strings
ClosedPublic

Authored by elexis on Sep 13 2018, 2:27 PM.

Details

Summary

rP8925 missed to initialize the m_ErrorBuffer with an empty string.
This leads to undefined data being passed to JS and displayed in the status caption.
Also curl_easy_strerror should be used if the error buffer wasn't written.

Test Plan

Shutdown your network interface, start the UserReporter.
You see a different caption with random data each time you start the unpatched code and a proper string with the patch.

Notice that curl_easy_strerror provided useful error strings here, because the error buffer was not written to.
https://curl.haxx.se/libcurl/c/CURLOPT_ERRORBUFFER.html

Notice that we probably don't want to translate 93 cURL error strings:
https://curl.haxx.se/libcurl/c/libcurl-errors.html#CURLEOK

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Sep 13 2018, 2:27 PM
Vulcan added a subscriber: Vulcan.Sep 13 2018, 2:33 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/732/display/redirect

Stan added a subscriber: Stan.Sep 13 2018, 7:27 PM
Stan added inline comments.
source/ps/UserReport.cpp
350 ↗(On Diff #6884)

Couldn't be m_ErrorBuffer = {0}
Or maybe better if it says something No error ?

elexis added inline comments.Sep 15 2018, 1:51 PM
source/ps/UserReport.cpp
350 ↗(On Diff #6884)

Couldn't be m_ErrorBuffer = {0}

http://giyf.com
It seems it would be the same as m_ErrorBuffer = {} and set all CURL_ERROR_SIZE characters to 0.
ModIo.cpp uses m_ErrorBuffer[0] = '\0';.
char* foo = "" is suggested sometimes but supposedly shouldn't even compile: https://dev.krzaq.cc/post/stop-assigning-string-literals-to-char-star-already/

Or maybe better if it says something No error ?

I rather display no error than "no error". Also it would lead to uglier code because the special string has to be tested for the in the condition added below.

I failed to figure out whether the string concatenation with unintialized data or the return value of the JS Interface function constitutes undefined behavior or whether that is just defined behavior returning indeterminate data.

http://www.cplusplus.com/reference/string/string/operator+/ says

If s is not a null-terminated character sequence, it causes undefined behavior.

But it doesn't say what s is. Also any data maybe considered a null-terminated character sequence.

Either way broken.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 23 2018, 2:20 AM
This revision was automatically updated to reflect the committed changes.
lyv added a subscriber: lyv.Sep 23 2018, 12:11 PM
lyv added inline comments.
ps/trunk/source/ps/UserReport.cpp
360

Not related to this revision proposal, but is it a good idea to not use a curl_multi for this? If the report get sufficently large, this could cause issues, no?

elexis added inline comments.Sep 23 2018, 4:10 PM
ps/trunk/source/ps/UserReport.cpp
360

The UserReporter is running in a separate thread, so it's not a problem to use the synchroneous / blocking curl easy handlers.
ModIo runs in the same thread and uses the non-blocking asynchroneous curl multi handlers.

lyv added inline comments.Sep 24 2018, 3:47 PM
ps/trunk/source/ps/UserReport.cpp
360

The UserReporter is running in a separate thread,

Ah, I see.