Re: RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr in getLastErrorString
On Sep 24, 3:10pm, rob.mcke...@oracle.com (Rob McKenna) wrote: -- Subject: Re: RFR - 8133249: Occasional SIGSEGV: non thread-safe use of str | Hi folks, | | Uploaded a newer version: | | http://cr.openjdk.java.net/~robm/8133249/webrev.02/ | | To address Rogers question, this needed to be separate from | getLastErrorString because of the return type and the err arg, but I | like the idea of avoiding the creation of a new file. Also, all of the | files that require the new function (now called getErrorString) already | include jni_util.c so it makes sense to put this in jni_util_md.c with | getLastErrorString. | | As per Christos' suggestion I've mapped strerr_r to __xpg_strerror_r | instead of using the gnu strerror_r. I'd be interested to hear feedback | on this change. | | I've corrected the oversights around the jli sources and | PlainDatagramSocket. The jli stuff shouldn't have been there in the | first place. | | Ivan, I've kept the return type. As you note, I should have been using | it in ProcessImpl_md.c and the extra information from the return is | potentially useful. LGTM. In src/java.base/windows/native/libnet/TwoStacksPlainDatagramSocketImpl.c shouldn't the sprintf() get changed to jio_snprintf() or at least snprintf()/_snprintf()? It would be nice if someone went and killed all the sprintfs()... christos
Re: RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr in getLastErrorString
On Sep 21, 4:53pm, rob.mcke...@oracle.com (Rob McKenna) wrote: -- Subject: RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr | Hi folks, | | Requesting a review of this change which switches corelibs usages of the | thread-unsafe strerror over to strerror_r/strerror_s: | | http://cr.openjdk.java.net/~robm/8133249/webrev.01/ I like this change, but in jdk_strerror.c: 1. Why are you using "errno" instead of "err" in the linux strerror_r() invocation? 2. Isn't the "Unknown error" test going to break on non-english locales? Best, christos
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
On May 22, 10:54am, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: RFR 9: 8074818: Resolve disabled warnings for libjava | I agree it's a good idea to increase safety by replacing calls to *printf | with calls to *nprintf, BUT when we do so we should also add debugging | assertions that the message fits into the buffer. | | -sprintf(errmsg, format, errnum, detail); | +snprintf(errmsg, fmtsize, IOE_FORMAT, errnum, detail); | | How about | | int needed = snprintf(...) | assert(needed = fmtsize); This only works if fmtsize is unsigned (which I hope it is) when snprintf returns 0. It will also produce a warning with -Wsign-compare. For safety you could do: assert((size_t)needed = fmtsize) christos
Re: stop using mmap for zip I/O
On Mar 3, 2:41pm, dmitry.samers...@oracle.com (Dmitry Samersoff) wrote:
-- Subject: Re: stop using mmap for zip I/O
| Christos,
|
| JVM install it's own signal handler and use it to numerous tasks, so add
| signal handler to zip_util.c is problematic.
Yes, I understand; this is why I mentioned that we not change it
until the pure java implementation is released.
| The simplest way to address the problem is use mlock/munlock to test
| page presence before touching it.
|
| i.e. add code like
|
| if ((i % 4096) == 0) {
| if ( mlock(v+i, 4096) 0) {
|printf(No page. Bail out\n);
|return;
| }
| }
|
| to *compute()* in your example below.
Yes, that fixes it too, at the cost of more syscalls and complexity...
I just mentioned disable mmap, because it is easy and fixes a common
problem without any performance penalties. Perhaps the mmap code made
sense when it was mapping the whole file, but now mapping just the
central directory doesn't. (IMHO :-)
christos
Re: stop using mmap for zip I/O
On Feb 27, 9:34am, a...@redhat.com (Andrew Haley) wrote: -- Subject: Re: stop using mmap for zip I/O | On 26/02/15 22:17, Christos Zoulas wrote: | There are numerous bug reports about the jvm crashing in libzip... | Just google for libzip java crash. The bottom line is that using | mmap is problematic (I can get into more per OS details if necessary) | because it will potentially signal when the file size is altered. | | So we catch the signal, right? Maybe there's something I'm missing... https://bugs.openjdk.java.net/browse/JDK-801 Uncomment the crashReadWrite() test, add a few imports and run it... [it crashes the jdk with SIGBUS on linux] christos
Re: stop using mmap for zip I/O
On Feb 27, 1:49pm, a...@redhat.com (Andrew Haley) wrote: -- Subject: Re: stop using mmap for zip I/O | The issues I've been looking at are SEGV issues concerning multiple threads | operating on the same zip structure. (one freeing, while another is | attempting access) Yes, I am not referring to the SEGV's from concurrent access or unchecked buffer limits from corrupt entries in the central directory. These are much more complicated to fix. | The issue Christos is referring to is probably the bus error you get | when a buffer is unmapped. This is what I was referring to as well. Yes, but the cause is not the buffer being unmapped. It is because the buffer gets truncated: http://pubs.opengroup.org/onlinepubs/009695399/functions/mmap.html References within the address range starting at pa and continuing for len bytes to whole pages following the end of an object shall result in delivery of a SIGBUS signal. Which is what my example program and the bug report I referenced demonstrate. | Recovering from a crash caused by one thread modifying a structure | while another reads it is much harder; my apologies, I misunderstood | you. Absolutely, I was just advocating turning mmap off since it fixes the simple case of someone overwriting the zip file loaded which is quite common. christos
Re: stop using mmap for zip I/O
On Feb 27, 9:51am, sean.cof...@oracle.com (=?windows-1252?Q?Se=E1n_Coffey?=) wrote: -- Subject: Re: stop using mmap for zip I/O | The sun.zip.disableMemoryMapping=true property helps with ZipFile class | only. There are other related issues in the ZipInput/Output streams. | Some code areas don't have synchronization and there are opportunities | for the underlying native zip resources to be freed which Java code then | tries to reference causing various SEGV. I've one or two areas of code | identified for tightening up and hope to get to it in coming weeks. | (long standing action item) Absolutely, there are unchecked lengths that are used to copy buffers... | So we catch the signal, right? Maybe there's something I'm missing... | We don't catch/detect such SEGV's. Would it make sense, is it possible ? It is SIGBUS... christos
stop using mmap for zip I/O
Hi,
There are numerous bug reports about the jvm crashing in libzip...
Just google for libzip java crash. The bottom line is that using
mmap is problematic (I can get into more per OS details if necessary)
because it will potentially signal when the file size is altered.
Can we please turn USE_MMAP off, and/or remove the code (zip_util.c)?
I don't think it is acceptable for the jvm to crash if it tries to
read a file while it is being modified. The following simple program
demonstrates the issue... just:
$ cc mmap.c
$ cp a.out b.out
$ ./a.out b.out
Best,
christos
$ cat _EOF mmap.c
#include stdio.h
#include stdlib.h
#include unistd.h
#include fcntl.h
#include err.h
#include signal.h
#include sys/mman.h
#include sys/stat.h
volatile size_t i;
size_t size = 0;
void
sig(int s)
{
printf(boom %d %zu\n, s, i);
exit(1);
}
void
compute(unsigned char *v)
{
int j = 0;
for (i = 0; i size; i++)
j += v[i];
printf(%d\n, j);
}
int
main(int argc, char *argv[])
{
struct stat st;
unsigned char *v;
int fd;
signal(SIGSEGV, sig);
signal(SIGBUS, sig);
fd = open(argv[1], O_RDONLY);
if (fd == -1)
err(1, open %s, argv[1]);
if (fstat(fd, st) == -1)
err(1, fstat %s, argv[1]);
size = st.st_size;
if (size == 0)
errx(1, 0 sized file);
v = mmap(0, size, PROT_READ, MAP_FILE | MAP_PRIVATE, fd, 0);
if (v == MAP_FAILED)
err(1, mmap);
printf(go1\n);
compute(v);
truncate(argv[1], 0);
printf(go2\n);
compute(v);
return 0;
}
_EOF
Re: stop using mmap for zip I/O
On Feb 26, 2:32pm, xueming.s...@oracle.com (Xueming Shen) wrote: -- Subject: Re: stop using mmap for zip I/O | That's true, unfortunately, the mmap does trigger crash here and there | | There is a property you can define to disable it in later releases | -Dsun.zip.disableMemoryMapping=true | | There is discussion that maybe we should simply disable the mmap by default ... | | And, there is further plan to simply move the general zip file handling up to | pure java, without any native code, as the webrev showed below. I'm still | hoping we can do it in jdk9 later, when everyone is not that busy and can | help codereview the change. (the webrev might be a little aged) Thanks a lot! The plan sounds great, as the pure java implementation will prevent other core-dumps from corrupt zip files... Best, christos
Re: RFR(S): JDK-8073175, 8073174: Fix native warnings in libjli and libfdlibm
On Feb 15, 6:52pm, dmitry.samers...@oracle.com (Dmitry Samersoff) wrote: -- Subject: RFR(S): JDK-8073175, 8073174: Fix native warnings in libjli and l | Hi Everyone, | | It's my $0.02 to the warning cleanup work. Please review: | | http://cr.openjdk.java.net/~dsamersoff/JDK-8073175/webrev.01 | http://cr.openjdk.java.net/~dsamersoff/JDK-8073174/webrev.01 | | Notes: | | 1. | There are two common ways to cast pointer to int: intptr_t (perfectly | safe, more-or-less portable) and size_t (perfectly portable, | more-or-less safe) | | So I use size_t in shared code, intptr_t in UNIX specific code. | Why are you using a signed and an unsigned type? You should probably be using uintptr_t for consistency. christos
Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses
On Feb 13, 2:52pm, dean.l...@oracle.com (Dean Long) wrote: -- Subject: Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer | My understanding is that whether or not aarch64 allows unaligned=20 | accesses is based on a | system setting, so this change is too simplistic. I would prefer that=20 | this was controlled with | something more flexible, like sun.cpu.unaligned. So does x86_64 and you can ask the CPU if it is enabled... I am not sure if a variable setting makes sense because if alignment is required you get a signal (BUS error -- hi linux, SEGV), or incorrect results. christos
Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer accesses
On Feb 13, 4:29pm, vladimir.koz...@oracle.com (Vladimir Kozlov) wrote: -- Subject: Re: RFR: 8073093: AARCH64: C2 generates poor code for ByteBuffer | On 2/13/15 4:22 PM, Dean Long wrote: | There is a system register bit to read, but I don't think it can be | accessed by an application, only the kernel. | If the OS won't provide this information, you could do something similar | to safeFetchN and catch the | resulting SIGBUS. | | Yes, I agree it could be done this way too. | On x86 we trigger SEGV to verify that OS's signal handler correctly | save/restore AVX registers so we can use them. It is PSL_AC (0x4) and it is accessible by applications. Now if it works or not depends on the flavor of the x86... As I mentioned before there are implementations (for example pre-arm-v6 flavors) where unaligned accesses don't signal (but don't work). There is an even 3rd category where unaligned accesses trap, but the kernel can fix them if the binary is marked specially (sparc with misaligned for example). The portable to verify what's going on is to do the misaligned access and see if it works (dealing with SIGBUS/SIGSEGV). Even then (even when it works) you might not want to do it because of performance reasons (for example when the kernel fixes it). christos
Re: 8072909: TimSort fails with ArrayIndexOutOfBoundsException on arrays longer than 1073741824
On Feb 12, 4:56pm, lev.pri...@oracle.com (Lev Priima) wrote: -- Subject: Re: 8072909: TimSort fails with ArrayIndexOutOfBoundsException on | Christos, | | Test may fail on shorter arrays(page 8 of paper). For instance, on worst | case, generated by test, it starts to fail on length 67108864. | After increasing stack size of runs to merge, Arrays.sort(T[]) works | also on maximum possible array for HotSpot JVM. | | | Roger, David, | I've updated the test ( | http://cr.openjdk.java.net/~lpriima/8072909/webrev.01/test/java/util/Arrays/TimSortStackSize2.java.html | ) to make it more suitable for regular execution: | |27 * @run main/othervm TimSortStackSize2 67108864 |28 * not for regular execution on all platforms: |29 * run main/othervm -Xmx8g TimSortStackSize2 1073741824 |30 * run main/othervm -Xmx32g TimSortStackSize2 2147483644 | | Could you please push this: | http://cr.openjdk.java.net/~lpriima/8072909/webrev.01/ | ? | Thanks for the explanation Lev! christos
Re: 8072909: TimSort fails with ArrayIndexOutOfBoundsException on arrays longer than 1073741824
On Feb 12, 9:57pm, david.hol...@oracle.com (David Holmes) wrote: -- Subject: Re: 8072909: TimSort fails with ArrayIndexOutOfBoundsException on | Ok - thanks Lev! | | David For posterity can someone document this, and also the value for which Integer.MAX_VALUE-4 fails? christos
Re: Losing features of JVM_Open, e.g. CLOEXEC
On Oct 30, 11:26am, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: Losing features of JVM_Open, e.g. CLOEXEC | On Thu, Oct 30, 2014 at 10:52 AM, Mario Torre | neugens.limasoftw...@gmail.com wrote: | Indeed, but /proc/$PID/fd can perhaps be useful here? Not sure if this | can make a reasonable test though. | | Mario, I'm not sure what you mean. | | Even if we can find and inspect each fd, and even if we knew the | stacktrace of where every fd was created, we still wouldn't know which | close-on-exec flag should be cleared. Let's not forget O_NOSIGPIPE, (and the equivalent SOCK_CLOEXEC and SOCK_NOSIGPIPE, for socket(2)). Also all the dup*(), pipe*(), kqueue*(), and fcntl() calls -- (in general all the system calls that can create file descriptors). christos
Re: Losing features of JVM_Open, e.g. CLOEXEC
On Oct 29, 1:12pm, marti...@google.com (Martin Buchholz) wrote: -- Subject: Losing features of JVM_Open, e.g. CLOEXEC | throwing away use of old shared infrastructure and replacing with naked | calls to the OS. Although understandable, this abandons benefits of using | shared infrastructure. Here I'm thinking of the close-on-exec flag. I | just added use of O_CLOEXEC to linux hotspot, but e.g. zip file opening no | longer uses JVM_Open, which is a code hygiene regression. | | What we want is to have almost all file descriptors have the close-on-exec | flag automatically set at fd creation time, using O_CLOEXEC where | available, and FD_CLOEXEC where not. How do we get there? | | I'm distressed that the JDK core libraries should be moving towards having | *more* shared native code infrastructure, but here we seem to be moving in | the opposite direction. Having abandoned JVM_Open, the responsibility of | doing these things right belongs entirely to the core libraries team. So | where's the core-library replacement for JVM_Open? I totally agree with Martin here. Changes like this are harmful. christos
Re: RFR [8020669] java.nio.file.Files.readAllBytes() does not read any data when Files.size() is 0
On Jul 22, 11:58am, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: RFR [8020669] java.nio.file.Files.readAllBytes() does not rea | It's the OS/filesystem implementer deciding to return a bogus number. | | More accurate would be to count by reading all the bytes, but that is | impractical and/or impossible (e.g. don't try reading all the bytes from | /dev/null) | | A middle ground would be to mistrust specifically file sizes of zero, | testing via | int c = read() | but then what? Check for /proc bug on Linux? The only sure way to read all the data from a file in Unix is to attempt to read(2) until it returns 0 (or -1 on error). If you think you can trust the size stat(2) returns what does it return for /dev/zero? :-) christos
Re: RFR [8020669] java.nio.file.Files.readAllBytes() does not read any data when Files.size() is 0
On Jul 23, 10:43am, david.ll...@redhat.com (David M. Lloyd) wrote: -- Subject: Re: RFR [8020669] java.nio.file.Files.readAllBytes() does not rea | The only sure way to read all the data from a file in Unix is to attempt | to read(2) until it returns 0 (or -1 on error). If you think you can trust | the size stat(2) returns what does it return for /dev/zero? :-) | | Indeed, that's a good point. It would probably be a good idea to | provide a variant that specifies a hard maximum number of bytes | (something less than 2GB anyway - the only thing worse than failing to | allocate such a large array is *succeeding* in allocating such a large | array when you don't want it). I guess one can always use a multiple of: blksize_t st_blksize preferred I/O block size (fs-specific) And if 0/unavailable, a multiple of page size. Letting the array grow too big will end up hurting performance anyway. christos
Re: RFR-8008118
On Apr 10, 11:54am, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: RFR-8008118 | 1. We did we switch from NEW() to xmalloc()? Why is the xmalloc cast | needed? | | NEW is for allocating homogeneous arrays, but here the memory block is | being used for both chars and pointers. I did not know that. Why the cast though? xmalloc() returns void *, no? Extraneous casts are bad because they hide conversion errors. For example, if you don't have the xmalloc prototype in scope, without the cast you get a warning of casting integer to pointer of different size. With the cast you get the wrong data assigned to the pointer. | 2. I would not declare pathv const char **, but char **, and then | cast the return if needed. This will make life easier in the future | if we decide to turn on warnings about const-castaways. | | | I believe the current code doesn't cast away const and doesn't write to | const. The only cast is to the return from xmalloc, which is expected. | What might a compiler warn about? It is casting away const before the memcpy: +p = (char *) pathv + pathvsize; Try to compile with -Wcast-qual. christos
Re: RFR-8008118
On Apr 10, 1:44pm, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: RFR-8008118 | Christos, I think you may have overlooked the subtlety that pathv is *not* | a const pointer - it just looks like one. | Yes, indeed; you are right! christos
Re: RFR-8008118
On Apr 2, 11:20pm, mark.shepp...@oracle.com (Mark Sheppard) wrote: -- Subject: Re: RFR-8008118 | Hi Martin, |are there any concerns that the string literal . might be on the | stack and not in data segment or heap? | In previous exchanges the static const char *const cwd was declared to | put it in read only memory. Constant string literal end up in the text or ro data segment. | Also, is it assured that the returned parentPathv will never be | de-allocated? I don't see parentPathv exposed beyond // | that of UNIXProcess_md.c, at least not directly. The parent can deal with freeing it I guess. christos
Re: RFR-8008118
On Mar 28, 8:42pm, marti...@google.com (Martin Buchholz) wrote:
-- Subject: Re: RFR-8008118
| Latest webrev does it this way:
|
| for (i = 0; i count; i++) {
| char *q = p + strcspn(p, :);
| pathv[i] = (p == q) ? . : p;
| *q = '\0';
| p = q + 1;
| }
I prefer to have loops where the sentinel is the actual condition instead
of something computed independently, but that's fine too.
christos
PS: Can we please add strsep(3) to solaris at some point? Everyone else
relevant seems to have it.
Re: RFR-8008118
On Mar 28, 11:12am, marti...@google.com (Martin Buchholz) wrote:
-- Subject: Re: RFR-8008118
| My apologies for unrelenting perfectionism - I noticed that we can:
| - colocate the pathv and path in one malloc'ed chunk
| - can discard xstrdup
| - can iterate through path with only one char* pointer.
|
| Making this code even cleaner, more concise and efficient.
-for (i = 0; i count; i++, p++) {
-pathv[i] = ((*p == ':') || (*p == '\0')) ? . : p;
-while (! ((*p == ':') || (*p == '\0')))
-p++;
-*p = '\0';
- }
- pathv[count] = NULL;
+ for (i = 0; (pathv[i] = strsep(p, :)) != NULL; i++)
+ if (!pathv[i][0])
+ pathv[i] = .;
christos
DefaultProxySelector socks override
This trivial patch add socksNonProxyHosts to the default proxy,
so that we can select which socket traffic will be directed to
the proxy and which not. There is currently no way to do this. In
my scenario, I have applications that would benefit in terms of
performance to connect directly to our internal network hosts, and
at the same time need to connect to the outside via our socks proxy.
Having all of them go through the socks proxy would require me to
buy a very expensive proxy, and suffer the latency anyway. I would
also like to note that the socksNonProxyHosts variable is in:
jdk/src/share/native/java/lang/System.c:PUTPROP(props,
socksNonProxyHosts, sprops-exceptionList);
for MacOS/X but nowhere else. Finally (not in this patch), it would
be nice to provide socks.nonProxyHosts etc. to be symmetric with the
other http, https, and ftp variables. But this is purely cosmetic. And
here's the patch...
Enjoy,
christos
--- jdk/src/share/classes/sun/net/spi/DefaultProxySelector.java.origWed Mar
27 10:26:36 2013 -0400
+++ jdk/src/share/classes/sun/net/spi/DefaultProxySelector.java Wed Mar 27
10:28:15 2013 -0400
@@ -124,6 +124,7 @@
final String defaultVal;
static NonProxyInfo ftpNonProxyInfo = new
NonProxyInfo(ftp.nonProxyHosts, null, null, defStringVal);
static NonProxyInfo httpNonProxyInfo = new
NonProxyInfo(http.nonProxyHosts, null, null, defStringVal);
+static NonProxyInfo socksNonProxyInfo = new
NonProxyInfo(socksNonProxyHosts, null, null, defStringVal);
NonProxyInfo(String p, String s, RegexpPool pool, String d) {
property = p;
@@ -186,7 +187,9 @@
pinfo = NonProxyInfo.httpNonProxyInfo;
} else if (ftp.equalsIgnoreCase(protocol)) {
pinfo = NonProxyInfo.ftpNonProxyInfo;
-}
+} else if (socket.equalsIgnoreCase(protocol)) {
+pinfo = NonProxyInfo.socksNonProxyInfo;
+}
/**
* Let's check the System properties for that protocol
Re: DefaultProxySelector socks override
On Mar 27, 3:54pm, chris.hega...@oracle.com (Chris Hegarty) wrote: -- Subject: Re: DefaultProxySelector socks override Hi Chris, | [cc'ing net-dev, we can then probably drop core-libs-dev and continue | the discussion over on net-dev] | | Christos, | | SOCKS is really old and not as widely deployed as other proxies. That | said, I don't have any specific problem with your proposal. SOCKS is | really in maintenance mode in the JDK, but I do see this as a reasonable | request/proposal. I totally understand; this is a legacy application for us too... | Since socksNonProxyHosts is only set on Mac I can only presume that it | is a remanent of the mac port. I would prefer to make the cosmetic | changes as part of this patch. I cannot see that we need to keep | socksNonProxyHosts, as it does nothing in the JDK anyway. | | Can you do this? Sure, I just requested a subscription to net-dev so I might not see the first few messages. To clarify: 1. I will add socks.proxyHost and socks.proxyPort for consistency with the other protocols, leaving as is socksProxyHost and socksProxyPort for compatibility. 2. I will add socks.nonProxyHosts and not socksNonProxyHosts. Is that what you had in mind? christos
Re: DefaultProxySelector socks override
On Mar 27, 5:30pm, chris.hega...@oracle.com (Chris Hegarty) wrote: -- Subject: Re: DefaultProxySelector socks override | On 03/27/2013 05:22 PM, chris...@zoulas.com wrote: | | Sure, I just requested a subscription to net-dev so I might not see the | first few messages. To clarify: | | 1. I will add socks.proxyHost and socks.proxyPort for consistency | with the other protocols, leaving as is socksProxyHost and | socksProxyPort for compatibility. | 2. I will add socks.nonProxyHosts and not socksNonProxyHosts. | | Is that what you had in mind? | | Re-checking the code I take back my previous comment. We already have | | socksProxyHost, socksProxyPort, socksProxyVersion | | so your original proposal of 'socksNonProxyHosts' is probably best, and | consistent with existing properties. I concur. Nothing for me to do :-) Best, christos
Re: RFR-8008118
On Mar 26, 6:13pm, marti...@google.com (Martin Buchholz) wrote:
-- Subject: Re: RFR-8008118
| Here is my alternative set of perfectionist changes:
:-)
| # Fix PATH handling
| http://cr.openjdk.java.net/~martin/webrevs/openjdk8/pathv/
LGTM, only question is why error from one allocation throws and from the
other does not? I would move the throw after the:
if (splitpath == NULL || pathv == NULL) {
This way we don't need to pass env to xstrdup(). Now you are going to make
me grep on how many xstrdup()'s are in the tree :-)
Correct and simple otherwise.
christos
Re: RFR-8008118
On Mar 21, 10:10am, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: RFR-8008118 | Please revert this formatting change: | | -for (q = p; (*q != ':') (*q != '\0'); q++) | -; | +for (q = p; (*q != ':') (*q != '\0'); q++); | + | Stylistically I prefer: for (q = p; (*q != ':') (*q != '\0'); q++) continue; so that re-formatting accidents don't happen, and the intent is clearly communicated. christos
Re: RFR-8008118
On Mar 21, 11:36am, john.zavg...@oracle.com (John Zavgren) wrote: -- Subject: Re: RFR-8008118 | All: | | How does this look? | 1.) I reverted the for statement formatting change. Not exactly. Now it is indented incorrectly. | 2.) I removed the goto statement and inlined some code instead. I prefer to write simpler code that works with both signed and unsigned indexes: +while (i 0) +if (pathv[--i] != cwd) +free(pathv[i]); + | 3.) I checked to make sure that we're not freeing memory that we didn't act= | ually allocate. (Path vector elements that are empty.) You are still using the ./ string literal constant in the code so you'll end up freeing it (the compiler will prolly merge the two instances and you'll get lucky but it is semantically incorrect). christos
Re: RFR: race with nested repos in hgforest.sh
On Feb 6, 10:04am, chris.hega...@oracle.com (Chris Hegarty) wrote:
-- Subject: Re: RFR: race with nested repos in hgforest.sh
| Yes this is better, but the quotes are incorrect. It should simply be
|for rc in ${tmp}/*.pid.rc ; do
This will break if there is no match.
christos
Re: bug fix for native kerberos libraries
On Oct 19, 9:11am, weijun.w...@oracle.com (Weijun Wang) wrote:
-- Subject: Re: bug fix for native kerberos libraries
| Hi Christos
|
| You mean the exception thrown in NativeGSSFactory.java lines 52-60?
|
| VectorGSSCredElement creds = GSSUtil.searchSubject
| (name, mech, initiate, GSSCredElement.class);
|
| // If Subject is present but no native creds available
| if (creds != null creds.isEmpty()) {
| if (GSSUtil.useSubjectCredsOnly(caller)) {
| throw new GSSException(GSSException.NO_CRED);
| }
| }
|
| Why would you leave GSSUtil.useSubjectCredsOnly to be true? IMHO, there
| is no need to call JGSS through JAAS when you are using a native provider.
Yes, I guess this is new with JDK 7. Thanks I will try that!
christos
bug fix for native kerberos libraries
Hello,
This simple fix allows kerberos authentication to work with:
-Dsun.security.jgss.native=true
and microsoft's sqljdbc 4.0.2206.100 driver.
Enjoy,
christos
--- a/java/src/sun/security/jgss/GSSUtil.java Mon Oct 15 17:43:08 2012 -0400
+++ b/java/src/sun/security/jgss/GSSUtil.java Mon Oct 15 17:44:28 2012 -0400
@@ -333,10 +333,19 @@
Subject accSubj = Subject.getSubject(acc);
VectorGSSCredentialSpi result = null;
if (accSubj != null) {
-result = new VectorGSSCredentialSpi();
IteratorGSSCredentialImpl iterator =
accSubj.getPrivateCredentials
(GSSCredentialImpl.class).iterator();
+// GSSCredentialImpl is only implemented in
+// the non-native kerberos implementation,
+// so if we don't get any elements here
+// assume native and return null so that
+// searchSubject does not fail. A better
+// fix is to implement the code that handles
+// this in native java.
+if (!iterator.hasNext())
+return null;
+result = new VectorGSSCredentialSpi();
while (iterator.hasNext()) {
GSSCredentialImpl cred = iterator.next();
debug(...Found cred + cred);
Re: StackTraceElement question
On Oct 8, 7:33pm, rednaxel...@gmail.com (Krystal Mok) wrote: -- Subject: Re: StackTraceElement question | Can't you just do Class.forName(getClassName()) and then find the | enclosing class? | | There could be potential class loader issues to | use Class.forName(getClassName()) in this case (most probably caused by | reflective calls). | But then again, giving the user a reference to instead of the name of a | class really gives the user more information then what's been given now | (e.g. class loader info). Which is not necessarily a good thing. I'd second | Alan on having to do more analysis. There is also the problem of having a class hierarchy like: class A extends class I class B extends class I and then trying to figure out if it is A or B when you just have I from that StackTraceElement. Alan is right, there could be security issues providing the class, and it is annoying and expensive to have to deal with them in the code that fills in StackTraceElement, and there could be also serialization issues. I think it is still a useful change though... christos
Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java.util, java.io
On Aug 30, 9:17am, a...@redhat.com (Andrew Haley) wrote: -- Subject: Re: Review Request: 7193406 - Clean-up JDK Build Warnings in java | -Werror is probably OK for Java code, but not for HotSpot. | | We GCC developers keep adding new warnings, so poor souls who keep | up-to-date with new Fedora (and other leading-edge) distributions tend | to be the first to discover problems. Thus is very awkward for people | who don't understand advanced C++ features. There are people who write code, and people who build the code to run/test binaries. The first group should use -Werror, the second should not. christos
Re: JDK 8 code review request for 7075098: Remove unused fdlibm files
On Aug 5, 10:41am, a...@redhat.com (Andrew Haley) wrote: -- Subject: Re: JDK 8 code review request for 7075098: Remove unused fdlibm f | On 08/04/2011 09:09 PM, Joe Darcy wrote: | | Mike Duigou wrote: | Looks good to me. | | I did wonder if we have a policy for tracking fdlibm updates. | | To a first approximation (and even a second approximation), fdlibm | doesn't have updates any more any hasn't for many years. The changes in | in 5.3 were to fix a few bugs I and others had discovered circa 2004. [1] | | That's right. We used fdlibm in libgcj too, and it is very solid and reliable | code. The only problems we ever had were some non-portable assumptions that | were fairly easy to fix. Maybe fdlibm will never now be updated. We are using fdlibm too on NetBSD (FreeBSD uses it too) and we had to fix a couple of bugs and add long double support. Perhaps we should all send our changes back and get a new release, or if they don't want to maintain it, we should maintain it centrally? christos
Re: Math.log(17197) produces inconsistent results between javac, jit, Math and StrictMath on various architectures.
On May 24, 4:55pm, xer...@zafena.se (=?UTF-8?B?WGVyeGVzIFLDpW5ieQ==?=) wrote: -- Subject: Re: Math.log(17197) produces inconsistent results between javac, | Im only able to test x86: | | But no javac still produces different ressult when compiling | | static double log_result = Math.log(log_value); | | and running javac on x86 and arm using the same version of hotspot: I have encountered this only on x86_64. x86 indeed works. The bug was introduced in jdk 1.6. christos
Re: Anyway in java to way for the child process to wait for parent process to die
On Nov 12, 8:29am, david.hol...@sun.com (David Holmes - Sun Microsystems)
wrote:
-- Subject: Re: Anyway in java to way for the child process to wait for paren
| Paulo Levi said the following on 11/12/09 05:31:
| In process builder.
|
| No.
|
| I'm not aware of any OS support for waiting for a parent process to die.
|
| David Holmes
All the BSD's have it.
christos
#include sys/types.h
#include sys/event.h
#include sys/time.h
#include stdio.h
#include unistd.h
#include err.h
int
main(int argc, char *argv[])
{
int kq, nev;
struct kevent ev, ch;
if ((kq = kqueue()) == -1)
err(1, Cannot create kqueue);
EV_SET(ch, getppid(), EVFILT_PROC, EV_ADD | EV_ENABLE | EV_CLEAR,
NOTE_EXIT, 0, 0);
nev = kevent(kq, ch, 1, ev, 1, NULL);
if (nev == -1)
err(1, kevent);
if (nev == 0)
errx(no event);
if (ev.fflags NOTE_EXIT)
printf(my parent is now %u\n, (unsigned)getppid());
else
errx(unknown flags %x, ev.fflags);
return;
}
Re: Review request for 5049299
On Jun 30, 4:39pm, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: Review request for 5049299 | In the JDK... | We close all non-relevant file descriptors in the child instead of relying | on | having the FD_CLOEXEC bit set properly for every fd in the parent. | | The technique of choice appears to be to read /proc/self/fd and | close all the fds found therein. | That's even portable between linux and solaris! Well, on solaris it is best to use closefrom(3C). I think some BSDs have it too, AIX, IRIX. So perhaps detect if it is there and use that instead ( or fcntl(fd, F_CLOSEM) if it exists. christos
Re: Review request for 5049299
On Jun 23, 3:33pm, a...@redhat.com (Andrew Haley) wrote: -- Subject: Re: Review request for 5049299 | I can debug this. | | Please try first syscall(SYS_clone ...) to bypass the libc gubbins. | That might be all you need. If that doesn't help I'll have a look. | | Isn't there some point at which you have to say to a Linux user Your | system is simply misconfigured. Fix the overcommit parameter and this | problem will go away ? Another thing to try is to add CLONE_VFORK to suspend the parent. christos
Re: Review request for 5049299
On May 26, 12:10pm, michael.mcma...@sun.com (Michael McMahon) wrote: -- Subject: Re: Review request for 5049299 | I played around with your test and I see similar behaviour on my Ubuntu | 8.04 system. | When you switch on always overcommit (not the default) then you get | the expected | behaviour, ie. you can go quite close to the VM limit, and manage to | spawn a small command | from Java. | | However, in the default heuristic mode, it doesn't work. The comment | on the web page | you mentioned says: | | Heuristic overcommit handling. Obvious overcommits of | address space are refused. Used for a typical system. It | ensures a seriously wild allocation fails while allowing | overcommit to reduce swap usage: | | Perhaps, this specifically does not suit the Java VM situation where | fork() of a very | large process, is considered an obvious overcommit. | | In that context, I'll look into using the clone() system call on Linux. | It seems | to be quite versatile and configurable. It looks likely that we can | avoid the use | of the helper command with it as well. Let's forget about the overcommit heuristics/settings alltogether. We need a portable solution to the problem that works in all scenarios. In addition the jvm maps memory on linux with MAP_NORESERVED which makes the situation even more complicated. Yes, overcommit is useful in a typical workstation setup, but not in a server environment where deterministic behavior is desired. christos
Re: Review request for 5049299
On May 26, 3:20pm, michael.mcma...@sun.com (Michael McMahon) wrote: -- Subject: Re: Review request for 5049299 | Christos Zoulas wrote: | Let's forget about the overcommit heuristics/settings alltogether. | We need a portable solution to the problem that works in all scenarios. | In addition the jvm maps memory on linux with MAP_NORESERVED which makes | the situation even more complicated. Yes, overcommit is useful | in a typical workstation setup, but not in a server environment where | deterministic behavior is desired. | | | Christos, | | Can you elaborate on what you have in mind? | I don't have a good solution. The only option I see to to use vfork(2) on systems that have it (*), and emulate it by suspending the parent manually on systems that don't in order to achieve the same process scheduling semantics. This of course takes away all the vfork() benefits (speed/efficiency, avoiding the memory exhaustion issues). This is not a good solution because the vfork() semantics vary across different OS's (specially with respect to signal handling) and we probably need to come up with a simple test program to see if the particular vfork() implementation is suitable, or we have to fail back to using fork. If there is a consensus to use vfork(): What is not clear to me is if java needs a special execute entry point that uses the fake or real vfork() (with the parent suspension semantics), or an implementation can just choose to implement execute using vfork() at it own digression. christos (*) On linux - for some earlier vintages that did not have a vfork() syscall - it is equivalent to calling clone(2) with flags CLONE_VFORK|CLONE_VM|SIGCHLD.
Re: SIGFPE with FPE_FLTRES
On Mar 25, 4:28pm, thomas.rodrig...@sun.com (Tom Rodriguez) wrote: -- Subject: Re: SIGFPE with FPE_FLTRES | That's even more odd to me. x86_64 shouldn't be using the old FP | instructions and the SSE based one don't produce an inexact traps as | far as I can tell. Maybe they are still being emitted somewhere, | possibly for the transcendentals? Actually I can see that the | template interpreter still uses them so maybe something is happening | there with a left over precision exception? | | tom I would just remove the trancendental c implementation. It is not consistent with the java one anyway on amd64: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6539464 It's been 2 years that this is has been open. I don't understand how come nobody else has hit this. My only guess is that people don't compare results from successive runs. christos
[PATCH]: Portability fixes
Hello,
Here are some simple changes for your consideration:
1. passing possibly negative values to isdigit is undefined behavior:
http://www.opengroup.org/onlinepubs/009695399/functions/isdigit.html
2. passing possibly negative values to isspace is undefined behavior:
http://www.opengroup.org/onlinepubs/009695399/functions/isspace.html
3. last is possibly uninitialized.
4. recvfrom argument should be socklen_t not int:
http://www.opengroup.org/onlinepubs/007908775/xns/recvfrom.html
Thanks,
christos
diff -r fc30e7f4b9b3 src/solaris/native/java/lang/UNIXProcess_md.c
--- a/src/solaris/native/java/lang/UNIXProcess_md.c Fri Jan 16 11:24:18
2009 -0500
+++ b/src/solaris/native/java/lang/UNIXProcess_md.c Mon Jan 22 16:25:36
2009 -0500
@@ -377,7 +377,7 @@
*/
while ((dirp = readdir(dp)) != NULL) {
int fd;
-if (isdigit(dirp-d_name[0])
+if (isdigit((unsigned char)dirp-d_name[0])
(fd = strtol(dirp-d_name, NULL, 10)) = from_fd + 2)
close(fd);
}
diff -r fc30e7f4b9b3 src/solaris/native/java/net/Inet4AddressImpl.c
--- a/src/solaris/native/java/net/Inet4AddressImpl.cFri Jan 16 11:24:18
2009 -0500
+++ b/src/solaris/native/java/net/Inet4AddressImpl.cMon Jan 22 16:25:36
2009 -0500
@@ -155,7 +155,7 @@
* Workaround for Solaris bug 4160367 - if a hostname contains a
* white space then 0.0.0.0 is returned
*/
-if (isspace(hostname[0])) {
+if (isspace((unsigned char)hostname[0])) {
JNU_ThrowByName(env, JNU_JAVANETPKG UnknownHostException,
(char *)hostname);
JNU_ReleaseStringPlatformChars(env, host, hostname);
@@ -172,7 +172,7 @@
return NULL;
} else {
int i = 0;
- struct addrinfo *itr, *last, *iterator = res;
+ struct addrinfo *itr, *last = NULL, *iterator = res;
while (iterator != NULL) {
int skip = 0;
itr = resNew;
@@ -603,7 +603,8 @@
ping4(JNIEnv *env, jint fd, struct sockaddr_in* him, jint timeout,
struct sockaddr_in* netif, jint ttl) {
jint size;
-jint n, len, hlen1, icmplen;
+jint n, hlen1, icmplen;
+socklen_t len;
char sendbuf[1500];
char recvbuf[1500];
struct icmp *icmp;
