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.

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

Fix segfault in NUT clients with empty host/device name components #2053

Merged
merged 6 commits into from
Sep 16, 2023
Merged

Fix segfault in NUT clients with empty host/device name components #2053

merged 6 commits into from
Sep 16, 2023

Conversation

jimklimov
Copy link
Member

Closes: #2052

The culprit was that in some libc implementations, calling strdup(NULL) causes a segfault, where such NULL can be returned by strtok() if there were no hits. Using xstrdup() introduced across NUT codebase specifically for safety checks like these turns it into a graceful failure that calling code can handle meaningfully.

See also #677 for related concerns.

@jimklimov jimklimov added bug portability We want NUT to build and run everywhere possible impacts-release-2.8.0 Issues reported against NUT release 2.8.0 (maybe vanilla or with minor packaging tweaks) impacts-release-2.7.4 Issues reported against NUT release 2.7.4 (maybe vanilla or with minor packaging tweaks) labels Sep 14, 2023
@jimklimov jimklimov added this to the 2.8.1 milestone Sep 14, 2023
@jimklimov jimklimov mentioned this pull request Sep 14, 2023
Closed
@AppVeyorBot

This comment was marked as resolved.

Sign in to view
@jimklimov
common/common.c: extend xbasename(), xstrdup() and xrealloc() definit…
05610d1 
…ion to return NULL if input was NULL (and log it) [networkupstools#2052]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov
clients/upsclient.c: fix strdup()=>xstrdup() to not segfault with bad…
fde7672 
… inputs [networkupstools#2052]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov
NEWS.adoc: update for issue networkupstools#2052 fix
048876f 
Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov
clients/upsclient.c: upscli_splitname(): add explicit checks for empt…
d2857f6 
…y upsname and/or hostname[:port] parts, to report problems as such [networkupstools#2052]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov
Copy link
Member Author

Testing:

:; make -j 20 -s && ( set -x; ./clients/upsc dev@localhost ; ./clients/upsc dev@bogushost; ./clients/upsc dev@; ./clients/upsc @host; ./clients/upsc @ )

+ ./clients/upsc dev@localhost
Error: Connection failure: Connection refused

+ ./clients/upsc dev@bogushost
Error: No such host

+ ./clients/upsc dev@
upscli_splitname: got the @ separator and then an empty hostname[:port] string
Error: invalid UPS definition.
Required format: upsname[@hostname[:port]]

+ ./clients/upsc @host
upscli_splitname: got empty upsname string
Error: invalid UPS definition.
Required format: upsname[@hostname[:port]]

+ ./clients/upsc @
upscli_splitname: got empty upsname string
Error: invalid UPS definition.
Required format: upsname[@hostname[:port]]

No more this report (from recent master) about trash going to networking name-resolver etc:

+ ./clients/upsc dev@localhost
Error: Connection failure: Connection refused

+ ./clients/upsc dev@bogushost
Error: No such host

+ ./clients/upsc dev@
upscli_splitaddr: can't parse empty string
Error: invalid UPS definition.
Required format: upsname[@hostname[:port]]

+ ./clients/upsc @host
Error: Unknown error

+ ./clients/upsc @
Segmentation fault

@jimklimov
Copy link
Member Author

jimklimov commented Sep 14, 2023 •
edited
Loading

Weird... a handful of builds got segfaults during make check-NIT, but in random-looking locations (gpiotest on debian, possibly upsd in Windows at https://ci.appveyor.com/project/nut-travis/nut/builds/48040434)

Gotta check if the xbasename() input-sanity fix here (and possibly those in xstrdup() etc.) misfired somehow...

@jimklimov
Revert "common/common.c: extend xbasename(), xstrdup() and xrealloc()…
05edad7 
… definition to return NULL if input was NULL (and log it) [networkupstools#2052]"

This reverts commit 05610d1.
Seems to cause segfaults on its own, maybe something relied on
older behavior (non-NULLs returned in case of bad inputs?)
To investigate separately later...
@jimklimov
common/common.c: extend xstrdup() definition to return NULL if input …
7c59cf1 
…was NULL (and log it) [networkupstools#2052]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@AppVeyorBot
Copy link

✅ Build nut 2.8.0.749-master completed (commit 871f926afd by @jimklimov)

@jimklimov jimklimov merged commit 484ed6d into networkupstools:master Sep 16, 2023
43 of 48 checks passed
@jimklimov jimklimov deleted the issue-2052 branch September 16, 2023 20:20
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
bug impacts-release-2.7.4 Issues reported against NUT release 2.7.4 (maybe vanilla or with minor packaging tweaks) impacts-release-2.8.0 Issues reported against NUT release 2.8.0 (maybe vanilla or with minor packaging tweaks) portability We want NUT to build and run everywhere possible
Projects
None yet
Milestone
2.8.1
Development

Successfully merging this pull request may close these issues.

passing null values to upsc results in kernel segmentation fault
2 participants
@jimklimov @AppVeyorBot