Skip to content

nut.exe are call upsdrvctl.exe in bin. #3065

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
+2 −2
Draft

nut.exe are call upsdrvctl.exe in bin. #3065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c495e65
nut.exe are call upsdrvctl.exe in bin.
Aug 21, 2025
da0075b
In case of WIN32 build,
Aug 21, 2025
97c01da
Merge branch 'master' into issue_3063_fix2
jimklimov Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/upsdrvctl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1652,7 +1652,7 @@ int main(int argc, char **argv)
#ifndef WIN32
driverpath = xstrdup(DRVPATH); /* set default */
#else /* WIN32 */
driverpath = getfullpath(NULL); /* Relative path in WIN32 */
driverpath = getfullpath(PATH_BIN); /* Relative path in WIN32 */
Copy link
Member

@jimklimov jimklimov Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nice, the method (in common.c) actually has a possible NULL deref problem if there is no backslash. And also looks only for that one, ignoring forward slashes (that recent Windows do accept... at least if not mixing two styles in same string - not sure yet what happens then).

  • UPDATE: Checked locally in cmd with dir "c:/temp\tools" - that worked okay in Win11 (as long as quotes were used, otherwise forward slash was assumed to start an invalid CLI argument)

This problem probably should never happen since we parse the output of WIN32 API method GetModuleFileName() but still that bit does not seem clean now that I looked at it.

Copy link
Member

@jimklimov jimklimov Sep 5, 2025

Choose a reason for hiding this comment

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

I think the whole PR is flawed here by assuming the drivers would be in PATH_BIN. This is a coincidence due to configure script arguments involved (or defaulted) in a particular build; the pedantically correct one would be the DRVPATH (or a relative path to that, starting from location of upsdrvctl.exe).

I believe here the root-cause problem is rather derived from assuming in this code spot that the tool is in same location as drivers (NULL relative path, compared to program module location), despite this drivers/Makefile.am piece:

# always build upsdrvctl
sbin_PROGRAMS = upsdrvctl

(because drivers are often hidden in some /lib/nut or under $libexec, and the tool to generally manage them should be in a more visible location).

The puzzle to solve here is in fact the relative path to drivers, I think, because the absolute paths built into the code may be flawed (in the default build config for Win32 in scripts/Windows/build-mingw-nut.sh script, the --prefix=/ and a shifted make install DESTDIR=$INSTALL_DIR is used).

#endif /* WIN32 */

atexit(exit_cleanup);
Expand Down
2 changes: 1 addition & 1 deletion scripts/Windows/wininit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ static DWORD run_drivers(void)
char command[NUT_PATH_MAX];
char *path;

path = getfullpath(PATH_BIN);
path = getfullpath(PATH_SBIN);
Copy link
Member

@jimklimov jimklimov Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Here the change to PATH_SBIN is correct (matching the install location of upsdrvctl(.exe) per drivers/Makefile.am).

Copy link
Member

@jimklimov jimklimov Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, it seems the relative path to another directory should be added with slash and dot-dot like "/../lib" (in one example); but "thanks" to --prefix=/ in those builds, I see paths actually almost ready to use with this approach - e.g.

$ grep PATH scripts/Windows/nut_build_x86_64-w64-mingw32/include/config.h
#define CONFIG_FLAGS "--host=x86_64-w64-mingw32 --build=x86_64-linux-gnu --prefix=/ --enable-keep_nut_report_feature PKG_CONFIG_PATH=/usr/x86_64-w64-mingw32/lib/pkgconfig --with-all=auto --with-doc='man=auto html-single=auto html-chunked=skip pdf=skip' --without-systemdsystemunitdir --with-pynut=app --with-augeas-lenses-dir=/augeas-lenses --enable-Werror"

#define ALTPIDPATH "/var/state/ups"
#define CGIPATH "//cgi-bin"
#define CONFPATH "//etc"
#define DRVPATH "//bin"
#define HTMLPATH "//html"
#define PIDPATH "/run"
#define STATEPATH "/var/state/ups"

...but lacking the /../ part - so now I am surprised it did find anything (whether binaries or configs).

Needs some more debugging I guess.

Also, the getfullpath() is used inconsistently I think, as it currently uses xstrdup() to return what it finds and needs the caller to free() the returned string, while methods that return PID, config, etc. locations generally return pointers to someone else's strings that must not be freed (env, built-in defaults). So I guess these calls can leak when repetitively called on Win32 builds.

if (nut_debug_level < 1) {
snprintf(command, sizeof(command), "%s\\upsdrvctl.exe start", path);
} else {
Expand Down
Loading