Skip to content

jsvc: Fix regression around invalid lockf(3) semantics #304

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

Closed
michael-o wants to merge 1 commit into from
+8 −8
Closed

jsvc: Fix regression around invalid lockf(3) semantics #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/changes/changes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
<body>
<release version="1.5.2" date="YYYY-MM-DD" description="Bug fix release">
<!-- FIX -->
<action type="fix" dev="michaelo">jsvc. Fix regression around invalid lockf(3) semantics.</action>
<!-- ADD -->
<!-- UPDATE -->
<action type="update" dev="ggregory" due-to="Gary Gregory">Bump org.apache.commons:commons-parent from 93 to 94.</action>
Expand Down
15 changes: 7 additions & 8 deletions src/native/unix/native/jsvc-unix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,6 @@ static int mkdir2(const char *name, int perms)
static int check_pid(arg_data *args)
{
int fd;
FILE *pidf;
char buff[80];
pid_t pidn = getpid();
int i, pid;
Expand Down Expand Up @@ -608,11 +607,11 @@ static int check_pid(arg_data *args)
return 122;
}
}
lseek(fd, SEEK_SET, 0);
pidf = fdopen(fd, "r+");
fprintf(pidf, "%d\n", (int)getpid());
fflush(pidf);
fclose(pidf);
i = snprintf(buff, sizeof(buff), "%d\n", (int)getpid());
lseek(fd, 0, SEEK_SET);
ftruncate(fd, 0);
write(fd, buff, i);
fsync(fd);
if (lockf(fd, F_ULOCK, 0)) {
log_error("check_pid: Failed to unlock PID file [%s] with file descriptor [%d] after reading due to [%d]",
args->pidf, fd, errno);
Expand Down Expand Up @@ -673,7 +672,7 @@ static int get_pidf(arg_data *args, bool quiet)
int i;
char buff[80];

fd = open(args->pidf, O_RDONLY, 0);
fd = open(args->pidf, O_RDWR, 0);
Copy link
Member

@garydgregory garydgregory Jan 5, 2026

Choose a reason for hiding this comment

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

Since this flag is used more than once, should it be extracted into a constant to ensure we show the intent of using the same value?

Copy link
Member Author

@michael-o michael-o Jan 5, 2026

Choose a reason for hiding this comment

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

You mean the zero? Or O_RDWR?

Copy link
Member

@garydgregory garydgregory Jan 5, 2026

Choose a reason for hiding this comment

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

I was talking about the flags parameter but I suppose the same could be said of the mode.

Copy link
Member Author

@michael-o michael-o Jan 5, 2026

Choose a reason for hiding this comment

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

I don't see a benefit adding a define for a define.

Copy link
Contributor

@sebbASF sebbASF Jan 7, 2026

Choose a reason for hiding this comment

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

Agreed, but I think the reason for allowing WRite should be documented in the code.

Possibly consider a private function to obtain the fd; this can then be fully documented.

if (!quiet)
log_debug("get_pidf: %d in %s", fd, args->pidf);
if (fd < 0) {
Expand Down Expand Up @@ -778,7 +777,7 @@ static int wait_child(arg_data *args, int pid)
}

/* check if the pid file process exists */
fd = open(args->pidf, O_RDONLY);
fd = open(args->pidf, O_RDWR);
if (fd < 0 && havejvm) {
/* something has gone wrong the JVM has stopped */
return 1;
Expand Down