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

Prevent potential issues and crashes with HTTP requests. #551

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Erlite
Copy link
Contributor

@Erlite Erlite commented Dec 21, 2022

Prevent demos from replaying HTTP requests, and fix a random crash that occurred, although I do think it's just memory corruption from a broken Northstar install on my end, the callstack is odd.

Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, compiles and runs just fine

@Erlite Erlite changed the title Prevent potential issues, fix crash. Prevent potential issues and crashes with HTTP requests. Dec 22, 2022
@BobTheBob9
Copy link
Member

not sure if this is necessarily a problem, but this being in script rather than native means that mods calling the internal functions directly for any reason would still play in demo, might be better to do this check in native?

@uniboi
Copy link
Contributor

uniboi commented Feb 27, 2023

Having this check in native would be more elegant but I think this is fine for now

@F1F7Y F1F7Y added needs testing Changes from the PR still need to be tested feedback wanted Feedback is wanted whether the changes by this PR are desired almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Apr 20, 2023
@uniboi
Copy link
Contributor

uniboi commented May 1, 2023

Thr native check is at client.dll+0x3900f3 but I haven't figured out how to deal with it yet

@ASpoonPlaysGames ASpoonPlaysGames removed the feedback wanted Feedback is wanted whether the changes by this PR are desired label Aug 25, 2023
@GeckoEidechse
Copy link
Member

bump

@@ -89,6 +89,7 @@ void function NSHandleSuccessfulHttpRequest( int handle, int statusCode, string
response.statusCode = statusCode
response.body = body
response.rawHeaders = headers
response.headers = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK @Erlite has become inactive on this account. Maybe @Zanieon could take this PR over?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't worked with anything on the HTTP requests of Northstar yet, judging by those functions code i'm uncertain how their logic functions, especially if there's integration at Launcher side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge needs testing Changes from the PR still need to be tested
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

8 participants