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

Fixed: NULL remoteHost in IOCTL_SO_INETATON #2503

Merged
merged 6 commits into from Jun 3, 2015

Conversation

sepalani
Copy link
Contributor

@sepalani sepalani commented Jun 2, 2015

Fix a crash when a domain name doesn't exist. (Ex: Monster Hunter Tri)

@Tilka
Copy link
Member

Tilka commented Jun 2, 2015

Not pretty, but that's not your fault... lgtm

{
Memory::Write_U32(Common::swap32(*(u32*)remoteHost->h_addr_list[0]), BufferOut);
INFO_LOG(WII_IPC_NET, "IOCTL_SO_INETATON = %d "
"%s, BufferIn: (%08x, %i), BufferOut: (%08x, %i), IP Found: %08X", remoteHost->h_addr_list[0] == nullptr ? -1 : 0,

This comment was marked as off-topic.

@sepalani
Copy link
Contributor Author

sepalani commented Jun 2, 2015

The last value can be set to INADDR_NONE (i.e. -1) when remoteHost is NULL. I'm not a huge fan of it because it's a valid broadcast address, but it's only for INFO_LOG so it should be fine.

@Tilka
Copy link
Member

Tilka commented Jun 2, 2015

Oh, and both the old and the current code dereference remoteHost->h_addr_list[0] and then check for null.

@BhaaLseN
Copy link
Member

BhaaLseN commented Jun 2, 2015

If you're doing a cleanup anyways, why not make it an if nullptr/else with two distinct logs?

@sepalani
Copy link
Contributor Author

sepalani commented Jun 2, 2015

There is no normal way for remoteHost->h_addr_list to be NULL in most Linux implementations:
#define h_addr h_addr_list[0] /* for backward compatibility */
I don't know if that's still the case with outlandish implementation so I checked it anyway.

Otherwise, making two distinct logs can be a good idea and it would be slightly faster than those ternaries I guess. What would you advise me to do?

@BhaaLseN
Copy link
Member

BhaaLseN commented Jun 2, 2015

Its unlikely to be noticably any faster or slower; my argument is for readibilty.

@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • line-width-test on dx-win-nv: diff

automated-fifoci-reporter

@sepalani
Copy link
Contributor Author

sepalani commented Jun 2, 2015

Fine.

Looks like my first commit but with more checks which allowed to get rid of those ternaries and unnecessary string formats by the same way.

However I don't know how this kind of changes can impact graphical rendering...

@Tilka
Copy link
Member

Tilka commented Jun 2, 2015

Ignore dx-win-nv, it's currently broken.

comex added a commit that referenced this pull request Jun 3, 2015
Fixed: NULL remoteHost in IOCTL_SO_INETATON
@comex comex merged commit f85db7a into dolphin-emu:master Jun 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants