Skip to content

who: fix --lookup #2210

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 1 commit into from
May 15, 2021
Merged

who: fix --lookup #2210

merged 1 commit into from
May 15, 2021

Conversation

jhscheer
Copy link
Contributor

@jhscheer jhscheer commented May 13, 2021 •
edited
Loading

This closes #2181.

who --lookup is failing with a runtime panic (double free).
Since crate::dns-lookup already includes a safe wrapper for getaddrinfo
I used this crate instead of further debugging the existing code in
utmpx::canon_host().

@jhscheer
Copy link
Contributor Author

jhscheer commented May 13, 2021 •
edited
Loading

It looks like socket2 which needs libc ^0.2.86 can't be compiled on CICD MInRustV.
(libc was constrained to <0.2.85 before this PR)

@sylvestre
Copy link
Contributor

can you try to use an older version of socket2?

@jhscheer
who: fix --lookup
a4fc2b5 
This closes uutils#2181.

`who --lookup` is failing with a runtime panic (double free).
Since `crate::dns-lookup` already includes a safe wrapper for `getaddrinfo`
I used this crate instead of further debugging the existing code in
utmpx::canon_host().

* It was neccessary to remove the version constraint for libc in uucore.
@jhscheer
Copy link
Contributor Author

The dependency issues are now fixed.

@sylvestre
Copy link
Contributor

Looking here https://github.com/uutils/coreutils/pull/2210/files
we can see that many lines aren't covered by tests, is that expected? thanks

@jhscheer
Copy link
Contributor Author

jhscheer commented May 14, 2021 •
edited
Loading

Since the code in question depend on who is logged in to the system and from where, it's hard to write tests for that.
However, the test for who --lookup which (in theory) reaches the lines the warning is complaining about, for utmpx.rs and for who.rs, is no longer set to ignore.
Also I added a test for pinky without flags which (in theory) reaches the lines the warning is complaining about.

One possibility would be to write a custom utmp file and point who to use that instead of the system's file.
(Other tests for uutils using utmpx.rs, i.e. users, uptime, pinky would also benefit from that approach.)
However, utmp is a binary file format and I don't know how feasible it is to work with it.

Another approach would be to move the function canon_host() out of utmpx.rs (would make sense anyway because imho thematically it doesn't belong there), modify it so it takes an argument, e.g. canon_host("172.217.16.142") and then write unit tests for this function.

@sylvestre sylvestre merged commit 620a5a5 into uutils:master May 15, 2021
@jtracey jtracey mentioned this pull request Feb 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"who --lookup" is failing on travis CI
2 participants
@jhscheer @sylvestre