8000 Fix Unix socket subsystem by cgfandia-tii · Pull Request #1181 · qilingframework/qiling · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix Unix socket subsystem #1181

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jul 19, 2022
Merged

Fix Unix socket subsystem #1181

merged 12 commits into from
Jul 19, 2022

Conversation

cgfandia-tii
Copy link
Contributor
@cgfandia-tii cgfandia-tii commented Jun 28, 2022 •
edited
Loading

Which kind of PR do you create?

  • This PR only contains minor fixes.
  • This PR contains major feature update.
  • This PR introduces a new function/api for Qiling Framework.

Coding convention?

  • The new code conforms to Qiling Framework naming convention.
  • The imports are arranged properly.
  • Essential comments are added.
  • The reference of the new code is pointed out.

Extra tests?

  • No extra tests are needed for this PR.
  • I have added enough tests for this PR.
  • Tests will be added after some discussion and review.

Changelog?

  • This PR doesn't need to update Changelog.
  • Changelog will be updated after some proper review.
  • Changelog has been updated in my PR.

Target branch?

  • The target branch is dev branch.

One last thing

xwings and others added 10 commits June 1, 2022 13:10
@xwings
Merge pull request qilingframework#1166 from qilingframework/dev
fd74c83 
Getting ready for 1.4.3
@cgfandia-tii
fix: ql_syscall_faccessat
76ae39e 
Dirty fix
@cgfandia-tii
fix: ql_syscall_writev
211dd07 
Remove `buf` decoding with `utf-8` encoding due to possible errors
@cgfandia-tii
fix: ql_syscall_connect, ql_syscall_bind
4964409 
Add support for the abstract socket namespace
@cgfandia-tii
fix: ql_unix_socket_path
1d271bf 
Remove leading null bytes from the path
@cgfandia-tii
fix: ql_syscall_sendto
59ecb1f 
Add `ql_unix_socket_path` for the `AF_UNIX`
@cgfandia-tii
fix: __getdents_common
0f07bb6 
`ino64_t` and `off64_t` are `unsigned long long` - not pointers
@cgfandia-tii
fix: ql_syscall_recvfrom
7b50666 
Add Unix abstract namespace handling
@cgfandia-tii
Merge pull request #1 from cgfandia-tii/master
a911e91 
Merge fixes
@cgfandia-tii
Merge branch 'qilingframework:dev' into dev
591976b
wtdcode
wtdcode reviewed Jun 28, 2022
View reviewed changes
qiling/os/posix/syscall/unistd.py Outdated
regreturn = 0
# TODO: according to real implementation, idk why here was a FIFO check
Copy link
Member

Choose a reason for hiding this comment

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

I track the history for this code. Looks like it's introduced here: 2f6ff43. Thus, it should be safe to remove the comments totatly.

elicn
elicn reviewed Jun 28, 2022
View reviewed changes
qiling/os/posix/syscall/unistd.py Outdated
@@ -732,7 +734,7 @@ def _type_mapping(ent):
return bytes([t])

if ql.os.fd[fd].tell() == 0:
n = ql.arch.pointersize
large_field_size = 8 if is_64 else 4
Copy link
Member
@elicn elicn Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Are you sure this is correct? According to the existing implementation n is set by the arch native pointer size, while is_64 indicates whether this function was called from ql_syscall_getdents or ql_syscall_getdents64 - not necessarily the arch bits.

I don't know if these are equivalent, and from the existing implementation it appears that they are not.
For example, ql_syscall_getdents64 may be called in a 32 bits environment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, large_field_size ( ino64_t and off64_t) is unsigned long long on x64 - not pointers, so the size of the structure linux_dirent64 does not depends on the architecture, but the initial implementation relies on the pointer size.

@cgfandia-tii
Copy link
Contributor Author
cgfandia-tii commented Jul 4, 2022 •
edited
Loading

Looks like during the test_elf_linux_x8664_getdents binary at examples/rootfs/x8664_linux/bin/x8664_getdents expects linux_dirent64, but readdir implementation of provided libc.so invokes getdents instead of getdents64.
This binary executed without problems with strace on my x64 host machine:

...
getdents64(3, /* 0 entries */, 32768)   = 0
write(1, "Open Success!\n", 14Open Success!
)         = 14
...

image

@elicn
Copy link
Member
elicn commented Jul 4, 2022

Looks like during the test_elf_linux_x8664_getdents binary at examples/rootfs/x8664_linux/bin/x8664_getdents expects linux_dirent64, but readdir implementation of provided libc.so invokes getdents instead of getdents64.

That aligns with my assumption that getdents64 or getdents might be called regardles of the arch used, and better rely on arch.pointersize rather than a flag coming from the caller. My guess is that reverting the changes made to the getdents implementation would solve this. I must admit that I didn't quite uderstand what had to be fixed there in the first place..

@cgfandia-tii
Copy link
Contributor Author

Looks like during the test_elf_linux_x8664_getdents binary at examples/rootfs/x8664_linux/bin/x8664_getdents expects linux_dirent64, but readdir implementation of provided libc.so invokes getdents instead of getdents64.

That aligns with my assumption that getdents64 or getdents might be called regardles of the arch used, and better rely on arch.pointersize rather than a flag coming from the caller. My guess is that reverting the changes made to the getdents implementation would solve this. I must admit that I didn't quite uderstand what had to be fixed there in the first place..

The cause of changing the implementation was a problem in the emulation of Android Armv7 binaries, where getdents64 was called with arch.pointersize == 4, which leads to incompatibility of the linux_dirent64 size. By the way, I will revert the changes, because I'm not sure that the problem in test binaries.

@elicn
Copy link
Member
elicn commented Jul 5, 2022 •
edited
Loading

The cause of changing the implementation was a problem in the emulation of Android Armv7 binaries, where getdents64 was called with arch.pointersize == 4, which leads to incompatibility of the linux_dirent64 size. By the way, I will revert the changes, because I'm not sure that the problem in test binaries.

Unless a malware sample, the binaries should be pretty standard so I'd assume they work properly. It's worth checking whether Android behaves differently than other POSIX here; if it does, then this method may be implemented specifically for Android: OS-specific syscalls implementation takes precesence over global POSIX syscalls'.

@cgfandia-tii cgfandia-tii requested a review from elicn July 10, 2022 20:17
@cgfandia-tii cgfandia-tii requested a review from wtdcode July 12, 2022 09:54
wtdcode
wtdcode approved these changes Jul 12, 2022
View reviewed changes
Copy link
Member
@wtdcode wtdcode left a comment

Choose a reas 8000 on for hiding this comment

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

Looks good to me.

@wtdcode
Copy link
Member
wtdcode commented Jul 12, 2022

Once CI is green, we are safe to go!

@xwings xwings merged commit 18eec6f into qilingframework:dev Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elicn elicn elicn approved these changes

@wtdcode wtdcode wtdcode approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cgfandia-tii @elicn @wtdcode @xwings
0