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

Implement GetUGCDetails #104

Merged
merged 1 commit into from
Dec 1, 2024
Merged

Implement GetUGCDetails #104

merged 1 commit into from
Dec 1, 2024

Conversation

Edremon
Copy link
Contributor

@Edremon Edremon commented Nov 27, 2024

This is needed in Half-Life 2 to get Workshop mods image.

@universal963 recommended the following change to avoid memory leak in case API consumer doesn't free ppchName.

diff --git a/dll/steam_remote_storage.cpp b/dll/steam_remote_storage.cpp
index babc96a5..9d0efb7f 100644
--- a/dll/steam_remote_storage.cpp
+++ b/dll/steam_remote_storage.cpp
@@ -512,10 +512,7 @@ bool Steam_Remote_Storage::GetUGCDetails( UGCHandle_t hContent, AppId_t *pnAppID
             ? mod.primaryFileSize
             : mod.previewFileSize;
 
-        if (ppchName) {
-            *ppchName = new char[mod_name.size() + 1];
-            std::strcpy(*ppchName, mod_name.c_str());
-        }
+        if (ppchName) *ppchName = (char*)mod_name.c_str();
         if (pnFileSizeInBytes) *pnFileSizeInBytes = mod_size;
         if (pSteamIDOwner) *pSteamIDOwner = mod.steamIDOwner;
 

I don't entirely agree, as this could result in dangling pointers. However, that seems to be how the Steam API works. I couldn't find any documentation stating whether the API consumer should copy the string or free it. I tried making that change with HL2, but it caused a segmentation fault in engine.so within SteamCallResults::runCallResults. We also sometimes get logs like [CWorkshopImage] Unknown image file type... UGC=0000000000000004. just before the segmentation fault.

@otavepto
Copy link
Contributor

wait what's causing seg fault? the pointer thing or the image path?

@Edremon
Copy link
Contributor Author

Edremon commented Nov 27, 2024

I'm not sure, I haven't reverse engineered engine.so, it happens inside it (at 0x0051780c).

   0xe895c7ec:	(bad)
   0xe895c7ed:	incl   -0x4a00f314(%ebx)
   0xe895c7f3:	pop    %eax
   0xe895c7f4:	(bad)
   0xe895c7f5:	(bad)
   0xe895c7f6:	ljmp   (bad)
   0xe895c7f7:	call   0xe897ea60
   0xe895c7fc:	mov    $0x4,%ecx
   0xe895c801:	mov    %eax,%edi
   0xe895c803:	add    $0x10,%esp
   0xe895c806:	lea    -0x22805a(%ebx),%esi
=> 0xe895c80c:	repz cmpsb %es:(%edi),%ds:(%esi)
   0xe895c80e:	seta   %dl
   0xe895c811:	sbb    $0x0,%dl
   0xe895c814:	test   %dl,%dl
   0xe895c816:	je     0xe895c970
   0xe895c81c:	mov    -0xc0(%ebp),%ebx
   0xe895c822:	mov    $0x4,%ecx
   0xe895c827:	mov    %eax,%edi
   0xe895c829:	lea    -0x1ed5bc(%ebx),%esi

@otavepto
Copy link
Contributor

otavepto commented Nov 27, 2024

Hmm can't reproduce on Windows, did this happen when you integrated #103 ? or was it happening before it ?
my suspicion is this line: https://github.com/Detanup01/gbe_fork/pull/103/files#diff-b0645ce20e2456012210f6c436d813f55ede57652d967b67a6e4f8dbeebd5e82L966

on Linux it must be file:///abs/path/img.png, but since the path is already expanded when it is relative, this becomes file:////abs.... (notice the 4 slashes)
on Windows absolute paths start like this C:\abs\path... so no change is needed.

if this seg fault started happening after you integrated #103 , revert that line and let me know.

@Edremon
Copy link
Contributor Author

Edremon commented Nov 27, 2024

The segfault is with @universal963 change, it works fine without it. Never tried without #103, but I doubt that interact with that, it's just not liking the (char*)mod_name.c_str() I guess.

@universal963
Copy link
Contributor

universal963 commented Nov 28, 2024

The segfault is with @universal963 change, it works fine without it. Never tried without #103, but I doubt that interact with that, it's just not liking the (char*)mod_name.c_str() I guess.

I've checked it again, the object mod_name is referenced to local object mod, resulting immediate object destruction when returning, thus invalidating the pointer too early. To resolve it, one of two possible approaches can be done:

  1. somehow refactor settings->getMod() to return a reference to private member Settings::mods. I personally think this could be a bit doing too much for such a little purpose.
  2. define mod_name as static std::string mod_name and copy the content to it, and pass the c_str() pointer to this static object out. The code may look not so nice but it just works.

Co-authored-by: a <e>
@Detanup01 Detanup01 merged commit 329bc3b into Detanup01:dev Dec 1, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants