Re: RFR: JDK-8293466: libjsig should ignore non-modifying sigaction calls
On Sat, 10 Sep 2022 06:15:05 GMT, Thomas Stuefe wrote:
> Found during code review of
> [JDK-8292695](https://bugs.openjdk.org/browse/JDK-8292695).
>
> We have two bugs in libjsig when we install hotspot signal handlers. Relevant
> code in libjsig:
>
>
> int sigaction(int sig, const struct sigaction *act, struct sigaction *oact) {
>
>
> sigused = sigismember(, sig);
> if (jvm_signal_installed && sigused) {
> /* jvm has installed its signal handler for this signal. */
> /* Save the handler. Don't really install it. */
> if (oact != NULL) {
> *oact = sact[sig];
> }
> if (act != NULL) {
> sact[sig] = *act;
> }
>
> signal_unlock();
> return 0;
> } else if (jvm_signal_installing) {
> /* jvm is installing its signal handlers. Install the new
> * handlers and save the old ones. */
> res = call_os_sigaction(sig, act, );
> sact[sig] = oldAct;
> if (oact != NULL) {
> *oact = oldAct;
> }
>
> /* Record the signals used by jvm. */
> sigaddset(, sig);
>
> signal_unlock();
> return res;
> }
>
> }
>
>
> Bug 1: we change state even if the sigaction call failed
> Bug 2: we change state even if the sigaction call was a non-modifying one
> (act == NULL)
>
> The latter is usually no problem since hotspot always calls `sigaction()` in
> pairs when installing a signal: first with NULL to get the old handler, then
> with the real handler. But this is not always true. If
> `AllowUserSignalHandlers` is set, and we find a custom handler is present, we
> will not override it:
>
>
> void set_signal_handler(int sig, bool do_check = true) {
> // Check for overwrite.
> struct sigaction oldAct;
> sigaction(sig, (struct sigaction*)NULL, ); < first sigaction
> call, libjsig now remembers signal as set
>
> // Query the current signal handler. Needs to be a separate operation
> // from installing a new handler since we need to honor
> AllowUserSignalHandlers.
> void* oldhand = get_signal_handler();
> if (!HANDLER_IS_IGN_OR_DFL(oldhand) &&
> !HANDLER_IS(oldhand, javaSignalHandler)) {
> if (AllowUserSignalHandlers) {
> // Do not overwrite; user takes responsibility to forward to us.
> return;
>
>
> That means:
> - we still have the original custom handler in place
> - but we already called sigaction, albeit with NULL, but libjsig now assumes
> that hotspot installed a handler itself.
>
> The result is that any further attempts to change the signal handler, whether
> by hotspot or by user code, will be prevented by libjsig. Any further
> non-modifying sigaction calls will return the original - still installed -
> custom handler.
>
> Admittedly, the error is very exotic. Users would have to set
> AllowUserSignalHandlers and preload libjsig, and *then* attempt to modify
> signal handlers after JVM initialization. But it is confusing, and a
> potential source for other errors. In hotspot, nobody counts on a
> non-modifying sigaction query changing program state somewhere.
>
> This seems to be an old bug, I see it in at least JDK 8. Did not look further
> into the past
>
> ---
>
> Tests: Ran the runtime/jsig and the runtime/Thread tests manually.
LGTM. Not an official OpenJDK Reviewer though.
-
Marked as reviewed by manc (Committer).
PR: https://git.openjdk.org/jdk/pull/10236
Re: Standard Artifact Resolution API
Okay so from what I can parse there are different streams of work here 1. The JDK could be well served by a "standard" build tool. I don't think this is controversial, but I'm nowhere near qualified enough to say what the "right" choice is. Maybe it is just taking gradle, who knows. What I do posit, though, is that the problem space is underexplored. I will claim this is partially because things like dependency resolution have been out of reach of the kinds of low stakes amateur exploration that drives innovation. The Node ecosystem churns through a build system a season, but they are at least actively exploring options - y'know? Let's call this hypothetical tool "jproject". 2. The JDK has the power to standardize improvements to dependency metadata and resolution The gradle team has done what they think is a good extension of the classifier system. There is some other analogous work in the rust ecosystem around features that could be interesting to incorporate. Maybe even ABI-level tool enforced minimum semver bumps like Elm, where we make something similar to Scala's Tasty metadata. Of all the players in the ecosystem, the JDK probably has the most power to make a given answer "happen", and being comfortable that the right answer has been reached may or may not be a blocker for "jproject" being real. 3. A standard dependency resolution api In order to successfully drive improvements to dependency resolution the JDK needs a tool, but I don't think that needs to be dependent on either knowing the "best-path-forward" metadata or deciding on a JDK build tool. There is enough prior work in tools like coursier (https://get-coursier.io/), shrinkwrap resolver (https://github.com/shrinkwrap/resolver), maven aether ( https://maven.apache.org/aether.html), and tools deps ( https://github.com/clojure/tools.deps.alpha) to at least start to talk about what a "jresolve" cli tool or "java.util.resolve"' api could look like. (apologies if that is only mildly coherent) On Sat, Sep 10, 2022 at 12:19 PM Scott Palmer wrote: > After thinking about it a bit more, I agree that a canonical dependency > format wouldn’t be much good without the tool to go with it. In my > particular case, I have builds that incorporate native code and Java. I > use Gradle partly because it handles both (though it could do a better job > with C and cross-compiling), and also because it can handle complex build > logic for putting the pieces together. One thing I dislike about tools > such as Rusts’ cargo or Node’s npm, is that they seem to make the > assumption that my project is based on a single language. That’s one reason > I don’t use Maven - it assumes you have a vanilla Java project and is > painful to use for anything beyond that. Any tool integrated into the JDK > should be able to build the JDK itself, including all the native bits, and > dealing with the multiple platforms and conditions. There should be no > need for external scripts to hack it together and very few external tools. > E.g. a C/C++ compiler would be needed to b build the JDK, but not > bash/Cygwin. > > I’ve often thought, why don’t they just use Gradle for building the JDK? > It seems to me that it would be so much simpler (and faster), but I can > imagine the mess of build-logic history that would need to be untangled. > I haven’t looked into alternatives like Bazel, as Gradle is working well > for me, but in the design of any tool for the JDK all of these other tools > should be looked at. Perhaps in the end the JDK could simply settle on one > of the modern tools already available and integrate or endorse it? That to > me would make more sense than building yet another tool ( > https://xkcd.com/927/). For example, if the JDK were to adopt Gradle as > the tool to use to build the JDK, it could de-facto become the “JDK build > tool”. Other projects formerly part of the JDK, like JavaFX, are already > using Gradle (though not as effectively as they could). > > Anyway, that’s just my 2 cents on the subject. > > Regards, > > Scott > > > On Sep 9, 2022, at 1:31 PM, Ethan McCue wrote: > > > We already have Ant+Ivy, Maven, Gradle and they have significantly > different philosophies such that agreeing on a single dependency management > tool may be too ambitious. > > Maybe it would be useful to dig into what those different philosophies are? > > > I suggest looking at what Gradle has done in this area. > > It would be a reasonable goal for Java to have a canonical format (like > Rust’s ‘cargo’ format) for external dependencies that addressed all the > issues and tools could use it and benefit from the potential cross-tool > compatibility. > > I don't disagree per-se, but without an actual tool the JDK doesn't > exactly have much leverage to drive adoption of whatever > dependency-metadata.[xml|json|toml|tar.gz] would address all the issues. It > also would still need to handle "the world as is" for published artifacts. > > > agreeing on a single
Re: RFR: 8279283 - BufferedInputStream should override transferTo [v7]
On Mon, 5 Sep 2022 18:33:43 GMT, Alan Bateman wrote:
>> Markus KARG has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> HexPrinter::transferTo
>
> It looks like this patch is against a repository that hasn't been sync'ed up
> since late 2021. BIS has changed, esp. the locking, so this is why you get
> the merge-conflict label. Look at the implXXX methods to see how the existing
> methods do the locking.
>
> I think I pointed out in one of the early rounds that you'll have to take the
> mark (if set) into account. It may be that you just call super.transferTo
> when markpos == -1.
>
> The other issue that needs consideration is that the draining of the buffered
> bytes will leak the underlying input stream to the target output stream. It
> may be safer to also call super.transferTo when (count - pos) > 0.
@AlanBateman I do not quite understand what would be wrong with the code below
instead of falling back to the super implementation *in case of non-empty
buffer*?
private long implTransferTo(OutputStream out) throws IOException {
if (getClass() == BufferedInputStream.class && markpos < 0) {
int avail = count - pos;
if (avail > 0) {
out.write(getBufIfOpen(), pos, avail);
pos = count;
}
return avail + getInIfOpen().transferTo(out);
} else {
return super.transferTo(out);
}
}
Can you please explain why you proposed to use the super implementation for
non-empty buffer? Just for my understanding. Thanks!
-
PR: https://git.openjdk.org/jdk/pull/6935
RFR: JDK-8293466: libjsig should ignore non-modifying sigaction calls
Found during code review of
[JDK-8292695](https://bugs.openjdk.org/browse/JDK-8292695).
We have two bugs in libjsig when we install hotspot signal handlers. Relevant
code in libjsig:
int sigaction(int sig, const struct sigaction *act, struct sigaction *oact) {
sigused = sigismember(, sig);
if (jvm_signal_installed && sigused) {
/* jvm has installed its signal handler for this signal. */
/* Save the handler. Don't really install it. */
if (oact != NULL) {
*oact = sact[sig];
}
if (act != NULL) {
sact[sig] = *act;
}
signal_unlock();
return 0;
} else if (jvm_signal_installing) {
/* jvm is installing its signal handlers. Install the new
* handlers and save the old ones. */
res = call_os_sigaction(sig, act, );
sact[sig] = oldAct;
if (oact != NULL) {
*oact = oldAct;
}
/* Record the signals used by jvm. */
sigaddset(, sig);
signal_unlock();
return res;
}
}
Bug 1: we change state even if the sigaction call failed
Bug 2: we change state even if the sigaction call was a non-modifying one (act
== NULL)
The latter is usually no problem since hotspot always calls `sigaction()` in
pairs when installing a signal: first with NULL to get the old handler, then
with the real handler. But this is not always true. If
`AllowUserSignalHandlers` is set, and we find a custom handler is present, we
will not override it:
void set_signal_handler(int sig, bool do_check = true) {
// Check for overwrite.
struct sigaction oldAct;
sigaction(sig, (struct sigaction*)NULL, ); < first sigaction call,
libjsig now remembers signal as set
// Query the current signal handler. Needs to be a separate operation
// from installing a new handler since we need to honor
AllowUserSignalHandlers.
void* oldhand = get_signal_handler();
if (!HANDLER_IS_IGN_OR_DFL(oldhand) &&
!HANDLER_IS(oldhand, javaSignalHandler)) {
if (AllowUserSignalHandlers) {
// Do not overwrite; user takes responsibility to forward to us.
return;
That means:
- we still have the original custom handler in place
- but we already called sigaction, albeit with NULL, but libjsig now assumes
that hotspot installed a handler itself.
The result is that any further attempts to change the signal handler, whether
by hotspot or by user code, will be prevented by libjsig. Any further
non-modifying sigaction calls will return the original - still installed -
custom handler.
Admittedly, the error is very exotic. Users would have to set
AllowUserSignalHandlers and preload libjsig, and *then* attempt to modify
signal handlers after JVM initialization. But it is confusing, and a potential
source for other errors. In hotspot, nobody counts on a non-modifying sigaction
query changing program state somewhere.
This seems to be an old bug, I see it in at least JDK 8. Did not look further
into the past
---
Tests: Ran the runtime/jsig and the runtime/Thread tests manually.
-
Commit messages:
- JDK-8293466-libjsig-should-ignore-non-modifying-sigaction-calls
- JDK-8293466-libjsig-should-ignore-non-modifying-sigaction-calls
Changes: https://git.openjdk.org/jdk/pull/10236/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=10236=00
Issue: https://bugs.openjdk.org/browse/JDK-8293466
Stats: 22 lines in 1 file changed: 14 ins; 3 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/10236.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/10236/head:pull/10236
PR: https://git.openjdk.org/jdk/pull/10236
Integrated: 8292240: CarrierThread.blocking not reset when spare not activated
On Thu, 11 Aug 2022 16:11:02 GMT, Alan Bateman wrote: > Some blocking operations that pin a virtual thread to its carrier will > compensate by activating a spare carrier (essentially managed blocker). This > is done by wrapping the operation in a Blocker begin/end. This code is not > correct for the case that a spare cannot be activated (e.g. already at the > max of 256 carrier threads). When that happens, the carrier thread's > "blocking" flag, used to detect reentrancy, is not reset when the blocking > operation is done. The result is that subsequent blocking operations by > virtual threads on this carrier thread will not attempt to activate a spare. > > The test runs with parallelism=1 and maxPoolSize=2 in order to make it easier > to create the conditions for this bug. This pull request has now been integrated. Changeset: 68da02c7 Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/68da02c7b536799ccca49e889c22f3e9a2691fb7 Stats: 154 lines in 2 files changed: 148 ins; 0 del; 6 mod 8292240: CarrierThread.blocking not reset when spare not activated Reviewed-by: dfuchs - PR: https://git.openjdk.org/jdk/pull/9841
