RFR(S): 8192978: Missing checks and small fixes in jdwp library

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S): 8192978: Missing checks and small fixes in jdwp library

Langer, Christoph

Hi,

 

I’d like to contribute a few fixes that have been done in our JDK builds that add a few additional checks and cleanups. Can I please get a review.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.0/

 

Thanks,

Christoph

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Chris Plummer
Hi Christoph,

Some of your changes could use explanations. Can you please elaborate on the problems you are fixing in the CR? Test cases, or at least explaining how to reproduce the issues you are fixing would also be useful.

shmemBase.c: seems correct, but could use explanation/example of when existing code was failing.

VirtualMachineImpl.c: When/why is pos ever null?

error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it?

error_messages.h: ok. looks like just whitespace cleanup

eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user?

inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it?

invoker.c: ok: looks like just whitespace cleanup

log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it?

I think you will need to file separate CRs for each issue. You can lump the whitespace cleanup into one. The rest probably should each be seaprate CRs, but I could see possibly doing just one CR for them if the CR documented each problem being fixed.

thanks,

Chris


On 12/4/17 7:44 AM, Langer, Christoph wrote:

Hi,

 

I’d like to contribute a few fixes that have been done in our JDK builds that add a few additional checks and cleanups. Can I please get a review.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.0/

 

Thanks,

Christoph

 


Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Langer, Christoph

Hi Chris,

 

thanks for looking at this.

 

The main part of the changes are the findings of a code scan tool (coverity). Here are some detailed comments. I also made a small update to the webrev (concerning eventHelper.c): http://cr.openjdk.java.net/~clanger/webrevs/8192978.1/.

 

 

> shmemBase.c: seems correct, but could use explanation/example of when existing code was failing.

 

This is a coverity finding. The correct size specification as per Microsoft’s doc [1] for jlong (__int64) would be I64.

VirtualMachineImpl.c: When/why is pos ever null?

 

This is also a coverity case. For some reason, coverity thinks that pos could be NULL here. But I’m with you – I also don’t see a path how that could ever happen. Nevertheless, this change would please converity and I don’t see it being performance critical so I would like to leave it. It’s also not wrong.

error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it?

 

This is a Windows only issue. “vsnprintf“ would write the terminating 0. But in jdk.jdwp.agent/windows/native/libjdwp/util_md.h, vsnprintf is redefined to _vsnprintf. And this, by the Mircrosoft documentation [2], doesn’t necessarily write the terminating character.

error_messages.h: ok. looks like just whitespace cleanup

 

yes

eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user?

 

For this one I don’t have the history. Maybe it was also the coverity tool. But there is no testcase. In my update I have updated 2 other places with the null check where bagAdd is called. I guess in case of such a null error occurring, it could help a little. But probably this would be something to be investigated by the developer rather than the debugging user.


inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it?

 

I found this fix in our code base. instream_endOfInput() is obviously never used. At least I don’t find any references in the source code scan. Maybe I shall completely remove it?

As for the “magic number of 11”: This is the offset to the payload of a jdwpCmdPacket, see jdk.jdwp.agent/share/native/include/ jdwpTransport.h.

    (sizeof(jint) /* len*/ + sizeof(jint) /* id */ + sizeof(jbyte) /* flags*/ + sizeof(jbyte) /* cmdSet */ + sizeof(jbyte) /*cmd */

Maybe I should write it down explicitly?

With that offset subtracted from the length field, we initialize stream->left with the correct value which makes the checks in readBytes (line 73) more correct.

But there is also no test case

invoker.c: ok: looks like just whitespace cleanup

 

Yes.

log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it?

 

Same goes as for error_messages.c.

 

 

I’m still hoping to get this change done under the hood of JDK-8192978. I will update the bug with some more details and also add an appropriate noreg label.

 

Best regards,

Christoph

[1] https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx

[2] https://msdn.microsoft.com/en-us/library/1kt27hek.aspx

 

 

From: Chris Plummer [mailto:[hidden email]]
Sent: Montag, 4. Dezember 2017 22:32
To: Langer, Christoph <[hidden email]>; [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

Some of your changes could use explanations. Can you please elaborate on the problems you are fixing in the CR? Test cases, or at least explaining how to reproduce the issues you are fixing would also be useful.

shmemBase.c: seems correct, but could use explanation/example of when existing code was failing.

VirtualMachineImpl.c: When/why is pos ever null?

error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it?

error_messages.h: ok. looks like just whitespace cleanup

eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user?

inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it?

invoker.c: ok: looks like just whitespace cleanup

log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it?

I think you will need to file separate CRs for each issue. You can lump the whitespace cleanup into one. The rest probably should each be seaprate CRs, but I could see possibly doing just one CR for them if the CR documented each problem being fixed.

thanks,

Chris


On 12/4/17 7:44 AM, Langer, Christoph wrote:

Hi,

 

I’d like to contribute a few fixes that have been done in our JDK builds that add a few additional checks and cleanups. Can I please get a review.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.0/

 

Thanks,

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Lindenmaier, Goetz

Hi Christoph,

 

thanks for doing this change.

I would appreciate if the coverity findings get into the codebase,

at minimum it simplifies repeated runs.

I’m not sure about the syntax cleanups, especially invoker.c,

this seems too trivial to touch the file.

 

Thanks for explaining inStream.c, I think it would help

documenting the number or computing it explicitly as you propose.

 

Best regards,

  Goetz.

 

From: serviceability-dev [mailto:[hidden email]] On Behalf Of Langer, Christoph
Sent: Dienstag, 5. Dezember 2017 11:52
To: Chris Plummer <[hidden email]>; [hidden email]
Subject: RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Chris,

 

thanks for looking at this.

 

The main part of the changes are the findings of a code scan tool (coverity). Here are some detailed comments. I also made a small update to the webrev (concerning eventHelper.c): http://cr.openjdk.java.net/~clanger/webrevs/8192978.1/.

 

 

> shmemBase.c: seems correct, but could use explanation/example of when existing code was failing.

 

This is a coverity finding. The correct size specification as per Microsoft’s doc [1] for jlong (__int64) would be I64.

VirtualMachineImpl.c: When/why is pos ever null?

 

This is also a coverity case. For some reason, coverity thinks that pos could be NULL here. But I’m with you – I also don’t see a path how that could ever happen. Nevertheless, this change would please converity and I don’t see it being performance critical so I would like to leave it. It’s also not wrong.

error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it?

 

This is a Windows only issue. “vsnprintf“ would write the terminating 0. But in jdk.jdwp.agent/windows/native/libjdwp/util_md.h, vsnprintf is redefined to _vsnprintf. And this, by the Mircrosoft documentation [2], doesn’t necessarily write the terminating character.

error_messages.h: ok. looks like just whitespace cleanup

 

yes

eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user?

 

For this one I don’t have the history. Maybe it was also the coverity tool. But there is no testcase. In my update I have updated 2 other places with the null check where bagAdd is called. I guess in case of such a null error occurring, it could help a little. But probably this would be something to be investigated by the developer rather than the debugging user.


inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it?

 

I found this fix in our code base. instream_endOfInput() is obviously never used. At least I don’t find any references in the source code scan. Maybe I shall completely remove it?

As for the “magic number of 11”: This is the offset to the payload of a jdwpCmdPacket, see jdk.jdwp.agent/share/native/include/ jdwpTransport.h.

    (sizeof(jint) /* len*/ + sizeof(jint) /* id */ + sizeof(jbyte) /* flags*/ + sizeof(jbyte) /* cmdSet */ + sizeof(jbyte) /*cmd */

Maybe I should write it down explicitly?

With that offset subtracted from the length field, we initialize stream->left with the correct value which makes the checks in readBytes (line 73) more correct.

But there is also no test case

invoker.c: ok: looks like just whitespace cleanup

 

Yes.

log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it?

 

Same goes as for error_messages.c.

 

 

I’m still hoping to get this change done under the hood of JDK-8192978. I will update the bug with some more details and also add an appropriate noreg label.

 

Best regards,

Christoph

[1] https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx

[2] https://msdn.microsoft.com/en-us/library/1kt27hek.aspx

 

 

From: Chris Plummer [[hidden email]]
Sent: Montag, 4. Dezember 2017 22:32
To: Langer, Christoph <[hidden email]>; [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

Some of your changes could use explanations. Can you please elaborate on the problems you are fixing in the CR? Test cases, or at least explaining how to reproduce the issues you are fixing would also be useful.

shmemBase.c: seems correct, but could use explanation/example of when existing code was failing.

VirtualMachineImpl.c: When/why is pos ever null?

error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it?

error_messages.h: ok. looks like just whitespace cleanup

eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user?

inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it?

invoker.c: ok: looks like just whitespace cleanup

log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it?

I think you will need to file separate CRs for each issue. You can lump the whitespace cleanup into one. The rest probably should each be seaprate CRs, but I could see possibly doing just one CR for them if the CR documented each problem being fixed.

thanks,

Chris


On 12/4/17 7:44 AM, Langer, Christoph wrote:

Hi,

 

I’d like to contribute a few fixes that have been done in our JDK builds that add a few additional checks and cleanups. Can I please get a review.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.0/

 

Thanks,

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Chris Plummer
In reply to this post by Langer, Christoph
Hi Christoph,

On 12/5/17 2:52 AM, Langer, Christoph wrote:

Hi Chris,

 

thanks for looking at this.

 

The main part of the changes are the findings of a code scan tool (coverity). Here are some detailed comments. I also made a small update to the webrev (concerning eventHelper.c): http://cr.openjdk.java.net/~clanger/webrevs/8192978.1/.

 

 

> shmemBase.c: seems correct, but could use explanation/example of when existing code was failing.

 

This is a coverity finding. The correct size specification as per Microsoft’s doc [1] for jlong (__int64) would be I64.

Yes. I was just wondering if using the incorrect format ever produced a test failure.  Also, isn't %ld incorrect for 32-bit platforms? But then I doubt this code has ever been compiled for a 32-bit unix target. Also, it looks like PRId64 should work for all platforms. This is what we use in hotspot to print a 64-bit value:

#define INT64_FORMAT           "%" PRId64

So maybe this fix can be simplified.


VirtualMachineImpl.c: When/why is pos ever null?

 

This is also a coverity case. For some reason, coverity thinks that pos could be NULL here. But I’m with you – I also don’t see a path how that could ever happen. Nevertheless, this change would please converity and I don’t see it being performance critical so I would like to leave it. It’s also not wrong.

I'm actually more concerned about making the code harder to read and misleading the reader into thinking it could actually be NULL. I'm not familiar with coverity, but I thought tools like this either provided a specific call path for these types of errors (in which case the problem might actually be up the call chain), or allowed you to configure the tool not to complain about specific cases that you know can't happen.


error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it?

 

This is a Windows only issue. “vsnprintf“ would write the terminating 0. But in jdk.jdwp.agent/windows/native/libjdwp/util_md.h, vsnprintf is redefined to _vsnprintf. And this, by the Mircrosoft documentation [2], doesn’t necessarily write the terminating character.

Any idea why we do this redefine of vsnprintf? In any case, it might be best to just unconditionally terminate rather than have WIN32 explicit code.


error_messages.h: ok. looks like just whitespace cleanup

 

yes

eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user?

 

For this one I don’t have the history. Maybe it was also the coverity tool. But there is no testcase. In my update I have updated 2 other places with the null check where bagAdd is called. I guess in case of such a null error occurring, it could help a little. But probably this would be something to be investigated by the developer rather than the debugging user.

I think you should hold off on this one until you have a better handle on why this was needed, and possible also include a test case.


inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it?

 

I found this fix in our code base. instream_endOfInput() is obviously never used. At least I don’t find any references in the source code scan. Maybe I shall completely remove it?

As for the “magic number of 11”: This is the offset to the payload of a jdwpCmdPacket, see jdk.jdwp.agent/share/native/include/ jdwpTransport.h.

    (sizeof(jint) /* len*/ + sizeof(jint) /* id */ + sizeof(jbyte) /* flags*/ + sizeof(jbyte) /* cmdSet */ + sizeof(jbyte) /*cmd */

Maybe I should write it down explicitly?

Can you use offsetof?

With that offset subtracted from the length field, we initialize stream->left with the correct value which makes the checks in readBytes (line 73) more correct.

But there is also no test case

Ok.


invoker.c: ok: looks like just whitespace cleanup

 

Yes.

log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it?

 

Same goes as for error_messages.c.

Ok.

 

 

I’m still hoping to get this change done under the hood of JDK-8192978. I will update the bug with some more details and also add an appropriate noreg label.

I think the coverity fixes can be one bug. The format changes one bug (I assume coverity did not find them). inStream.c should have its own bug. eventHelper.c should have its own bug and more investigation as to why it is needed. I get nervous when the reason for a change has been forgotten.

thanks,

Chris

 

Best regards,

Christoph

[1] https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx

[2] https://msdn.microsoft.com/en-us/library/1kt27hek.aspx

 

 

From: Chris Plummer [[hidden email]]
Sent: Montag, 4. Dezember 2017 22:32
To: Langer, Christoph [hidden email]; [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

Some of your changes could use explanations. Can you please elaborate on the problems you are fixing in the CR? Test cases, or at least explaining how to reproduce the issues you are fixing would also be useful.

shmemBase.c: seems correct, but could use explanation/example of when existing code was failing.

VirtualMachineImpl.c: When/why is pos ever null?

error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it?

error_messages.h: ok. looks like just whitespace cleanup

eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user?

inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it?

invoker.c: ok: looks like just whitespace cleanup

log_messages.c: is there any reason not to do this fix for all platforms? Do you have a test case for it?

I think you will need to file separate CRs for each issue. You can lump the whitespace cleanup into one. The rest probably should each be seaprate CRs, but I could see possibly doing just one CR for them if the CR documented each problem being fixed.

thanks,

Chris


On 12/4/17 7:44 AM, Langer, Christoph wrote:

Hi,

 

I’d like to contribute a few fixes that have been done in our JDK builds that add a few additional checks and cleanups. Can I please get a review.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

WebRev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.0/

 

Thanks,

Christoph

 

 


Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Langer, Christoph

Hi Chris,

 

thanks again for your reply.

 

 

> shmemBase.c: seems correct, but could use explanation/example of when existing code was failing.

 

 

This is a coverity finding. The correct size specification as per Microsoft’s doc [1] for jlong (__int64) would be I64.

Yes. I was just wondering if using the incorrect format ever produced a test failure.  Also, isn't %ld incorrect for 32-bit platforms? But then I doubt this code has ever been compiled for a 32-bit unix target. Also, it looks like PRId64 should work for all platforms. This is what we use in hotspot to print a 64-bit value:



#define INT64_FORMAT           "%" PRId64

So maybe this fix can be simplified.

 

You are probably right, I’ll address this in my next webrev.


VirtualMachineImpl.c: When/why is pos ever null?

 

This is also a coverity case. For some reason, coverity thinks that pos could be NULL here. But I’m with you – I also don’t see a path how that could ever happen. Nevertheless, this change would please converity and I don’t see it being performance critical so I would like to leave it. It’s also not wrong.

I'm actually more concerned about making the code harder to read and misleading the reader into thinking it could actually be NULL. I'm not familiar with coverity, but I thought tools like this either provided a specific call path for these types of errors (in which case the problem might actually be up the call chain), or allowed you to configure the tool not to complain about specific cases that you know can't happen.

 

OK, what I’m gonna do now: I’ve reverted the stuff in our codebase to get a new coverity run. If coverity still complains I’ll check again to first of all get an explanation by coverity what should be wrong and then find a more sensible fix. Or maybe we can tell coverity to shut up…


error_messages.c: Is there any reason not to do this fix for all platforms? Do you have a test case for it?

 

This is a Windows only issue. “vsnprintf“ would write the terminating 0. But in jdk.jdwp.agent/windows/native/libjdwp/util_md.h, vsnprintf is redefined to _vsnprintf. And this, by the Mircrosoft documentation [2], doesn’t necessarily write the terminating character.

Any idea why we do this redefine of vsnprintf? In any case, it might be best to just unconditionally terminate rather than have WIN32 explicit code.

 

After reading the specs again, I think that windows uses _vsnprintf since this version matches the standard Unix/Linux behavior better which means that the buffer is written up to count and a terminating 0 might not be written. So it’s best to terminate the buffer in any case which will be reflected by my new webrev.


eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message going to be of use to the user?

 

For this one I don’t have the history. Maybe it was also the coverity tool. But there is no testcase. In my update I have updated 2 other places with the null check where bagAdd is called. I guess in case of such a null error occurring, it could help a little. But probably this would be something to be investigated by the developer rather than the debugging user.

I think you should hold off on this one until you have a better handle on why this was needed, and possible also include a test case.

 

This one is coverity, too. I’m now doing the same approach like I do with VirtualMachineImpl.c. I’m removing the stuff and see if coverity will complain again and then rethink the patch.


inStream.c: inStream_init() now uses a magic number of 11. What does it represent? inStream_endOfInput() looks like it used to be completely broken. How was this not noticed? Is this related to the change inStream_init(), which possible was masking this error? Is there a test case for it?

 

I found this fix in our code base. instream_endOfInput() is obviously never used. At least I don’t find any references in the source code scan. Maybe I shall completely remove it?

As for the “magic number of 11”: This is the offset to the payload of a jdwpCmdPacket, see jdk.jdwp.agent/share/native/include/ jdwpTransport.h.

    (sizeof(jint) /* len*/ + sizeof(jint) /* id */ + sizeof(jbyte) /* flags*/ + sizeof(jbyte) /* cmdSet */ + sizeof(jbyte) /*cmd */

Maybe I should write it down explicitly?

Can you use offsetof?

 

Yes, will do in next webrev.

 

I’m still hoping to get this change done under the hood of JDK-8192978. I will update the bug with some more details and also add an appropriate noreg label.

I think the coverity fixes can be one bug. The format changes one bug (I assume coverity did not find them). inStream.c should have its own bug. eventHelper.c should have its own bug and more investigation as to why it is needed. I get nervous when the reason for a change has been forgotten.

Ok, I will eventually split as suggested.

 

So, I’ll check the coverity results tomorrow and then come up with new webrevs.

 

Best regards,

Christoph

 

Reply | Threaded
Open this post in threaded view
|

RFR(S): 8192978: Missing checks and small fixes in jdwp library

Langer, Christoph
In reply to this post by Langer, Christoph

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.


src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

 

Thanks in advance & Best regards

Christoph

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Chris Plummer
Hi Christoph,

On 12/8/17 5:48 AM, Langer, Christoph wrote:

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

If I understand correctly how the code currently works, pos/psPos will be NULL on the last iteration of the loop, but we'll exit anyway since "i == npaths" by that point. So the NULL pos will never be referenced. I guess coverity is cluing in on the following section:

 846         psPos = strchr(pos, ps[0]);
 847         if ( psPos == NULL ) {
 848             plen = (int)strlen(pos);
 849         } else {
 850             plen = (int)(psPos-pos);
 851             psPos++;
 852         }

We admit in line 847 that psPos can be NULL, but coverity can't read into the loop logic deep enough to recognize we'll also exit when this happens.

I guess your fix is fine, although technically unnecessary.


 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.


src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.

Looks good. Thanks for looking deeper into this one.


src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.

 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

Please list the files with just whitespace changes in the CR.

 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

I'm doing a pretty comprehensive round of serviceability testing myself with these changes and also 8193258. I'll let you know if there are any issues.

thanks,

Chris

 

Thanks in advance & Best regards

Christoph

 


Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Langer, Christoph

Thanks in advance, Chris, for testing.

 

Please let me know the outcome when you are done.

 

Best regards

Christoph

 

From: Chris Plummer [mailto:[hidden email]]
Sent: Freitag, 8. Dezember 2017 21:52
To: Langer, Christoph <[hidden email]>; [hidden email]
Cc: Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

On 12/8/17 5:48 AM, Langer, Christoph wrote:

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

If I understand correctly how the code currently works, pos/psPos will be NULL on the last iteration of the loop, but we'll exit anyway since "i == npaths" by that point. So the NULL pos will never be referenced. I guess coverity is cluing in on the following section:

 846         psPos = strchr(pos, ps[0]);
 847         if ( psPos == NULL ) {
 848             plen = (int)strlen(pos);
 849         } else {
 850             plen = (int)(psPos-pos);
 851             psPos++;
 852         }

We admit in line 847 that psPos can be NULL, but coverity can't read into the loop logic deep enough to recognize we'll also exit when this happens.

I guess your fix is fine, although technically unnecessary.




 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.



src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.

Looks good. Thanks for looking deeper into this one.


src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.

 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

Please list the files with just whitespace changes in the CR.


 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

I'm doing a pretty comprehensive round of serviceability testing myself with these changes and also 8193258. I'll let you know if there are any issues.

thanks,

Chris

 

Thanks in advance & Best regards

Christoph

 

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Chris Plummer
All testing passed.

thanks,

Chris

On 12/8/17 1:07 PM, Langer, Christoph wrote:

Thanks in advance, Chris, for testing.

 

Please let me know the outcome when you are done.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Freitag, 8. Dezember 2017 21:52
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

On 12/8/17 5:48 AM, Langer, Christoph wrote:

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

If I understand correctly how the code currently works, pos/psPos will be NULL on the last iteration of the loop, but we'll exit anyway since "i == npaths" by that point. So the NULL pos will never be referenced. I guess coverity is cluing in on the following section:

 846         psPos = strchr(pos, ps[0]);
 847         if ( psPos == NULL ) {
 848             plen = (int)strlen(pos);
 849         } else {
 850             plen = (int)(psPos-pos);
 851             psPos++;
 852         }

We admit in line 847 that psPos can be NULL, but coverity can't read into the loop logic deep enough to recognize we'll also exit when this happens.

I guess your fix is fine, although technically unnecessary.




 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.



src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.

Looks good. Thanks for looking deeper into this one.


src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.

 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

Please list the files with just whitespace changes in the CR.


 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

I'm doing a pretty comprehensive round of serviceability testing myself with these changes and also 8193258. I'll let you know if there are any issues.

thanks,

Chris

 

Thanks in advance & Best regards

Christoph

 

 


Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

serguei.spitsyn@oracle.com
In reply to this post by Langer, Christoph
Hi Christoph,

This looks good.
Please, do not forget to run the jtreg jdk/com/sun/jdi tests for fixes in the debugger agent library.

Thanks,
Serguei


On 12/8/17 05:48, Langer, Christoph wrote:

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.


src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

 

Thanks in advance & Best regards

Christoph

 


Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Langer, Christoph
In reply to this post by Chris Plummer

Hi Chris,

 

thanks for testing. I added the files with whitespace changes to the bug. I’ll push this on Monday.

 

Best regards

Christoph

 

From: Chris Plummer [mailto:[hidden email]]
Sent: Samstag, 9. Dezember 2017 01:18
To: Langer, Christoph <[hidden email]>; [hidden email]
Cc: Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

All testing passed.

thanks,

Chris

On 12/8/17 1:07 PM, Langer, Christoph wrote:

Thanks in advance, Chris, for testing.

 

Please let me know the outcome when you are done.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Freitag, 8. Dezember 2017 21:52
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

On 12/8/17 5:48 AM, Langer, Christoph wrote:

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

If I understand correctly how the code currently works, pos/psPos will be NULL on the last iteration of the loop, but we'll exit anyway since "i == npaths" by that point. So the NULL pos will never be referenced. I guess coverity is cluing in on the following section:

 846         psPos = strchr(pos, ps[0]);
 847         if ( psPos == NULL ) {
 848             plen = (int)strlen(pos);
 849         } else {
 850             plen = (int)(psPos-pos);
 851             psPos++;
 852         }

We admit in line 847 that psPos can be NULL, but coverity can't read into the loop logic deep enough to recognize we'll also exit when this happens.

I guess your fix is fine, although technically unnecessary.






 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.





src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.

Looks good. Thanks for looking deeper into this one.



src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.


 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

Please list the files with just whitespace changes in the CR.



 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

I'm doing a pretty comprehensive round of serviceability testing myself with these changes and also 8193258. I'll let you know if there are any issues.

thanks,

Chris

 

Thanks in advance & Best regards

Christoph

 

 

 

Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Langer, Christoph

Hi guys,

 

I just pushed this: http://hg.openjdk.java.net/jdk/jdk/rev/8db54e2c453b

 

However, for some reason somebody set my original bug to Fix version 11 which I kinda ignored and now the system created a backport bug for 10 (https://bugs.openjdk.java.net/browse/JDK-8193305). Probably I should have corrected the fix version before I pushed. Now, shall I set the original bug https://bugs.openjdk.java.net/browse/JDK-8192978 to “Resolved” manually or wait for the system to do it? Any hints on that one?

 

Thanks

Christoph

 

From: Langer, Christoph
Sent: Samstag, 9. Dezember 2017 18:08
To: 'Chris Plummer' <[hidden email]>; [hidden email]
Cc: '[hidden email]' <[hidden email]>
Subject: RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Chris,

 

thanks for testing. I added the files with whitespace changes to the bug. I’ll push this on Monday.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Samstag, 9. Dezember 2017 01:18
To: Langer, Christoph <[hidden email]>; [hidden email]
Cc: Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

All testing passed.

thanks,

Chris

On 12/8/17 1:07 PM, Langer, Christoph wrote:

Thanks in advance, Chris, for testing.

 

Please let me know the outcome when you are done.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Freitag, 8. Dezember 2017 21:52
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

On 12/8/17 5:48 AM, Langer, Christoph wrote:

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

If I understand correctly how the code currently works, pos/psPos will be NULL on the last iteration of the loop, but we'll exit anyway since "i == npaths" by that point. So the NULL pos will never be referenced. I guess coverity is cluing in on the following section:

 846         psPos = strchr(pos, ps[0]);
 847         if ( psPos == NULL ) {
 848             plen = (int)strlen(pos);
 849         } else {
 850             plen = (int)(psPos-pos);
 851             psPos++;
 852         }

We admit in line 847 that psPos can be NULL, but coverity can't read into the loop logic deep enough to recognize we'll also exit when this happens.

I guess your fix is fine, although technically unnecessary.




 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.



src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.

Looks good. Thanks for looking deeper into this one.


src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.

 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

Please list the files with just whitespace changes in the CR.


 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

I'm doing a pretty comprehensive round of serviceability testing myself with these changes and also 8193258. I'll let you know if there are any issues.

thanks,

Chris

 

Thanks in advance & Best regards

Christoph

 

 

 

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Chris Plummer
Hi Christoph,

I was just guilty of the same thing on Friday. Dan cleaned it up for me. See JDK-8191229. I believe the process is to change the Fix Version in the backport from 10 to 11, re-open it, and then close as resolved "No an issue" (with a comment as to why this was done). For the main CR, change the Fix Version from 11 to 10, copy the HG Updater info from the backport, close as resolved "Fixed". I think you also need to set "Resolved In Build" to "team".

Chris

On 12/10/17 11:39 PM, Langer, Christoph wrote:

Hi guys,

 

I just pushed this: http://hg.openjdk.java.net/jdk/jdk/rev/8db54e2c453b

 

However, for some reason somebody set my original bug to Fix version 11 which I kinda ignored and now the system created a backport bug for 10 (https://bugs.openjdk.java.net/browse/JDK-8193305). Probably I should have corrected the fix version before I pushed. Now, shall I set the original bug https://bugs.openjdk.java.net/browse/JDK-8192978 to “Resolved” manually or wait for the system to do it? Any hints on that one?

 

Thanks

Christoph

 

From: Langer, Christoph
Sent: Samstag, 9. Dezember 2017 18:08
To: 'Chris Plummer' [hidden email]; [hidden email]
Cc: '[hidden email]' [hidden email]
Subject: RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Chris,

 

thanks for testing. I added the files with whitespace changes to the bug. I’ll push this on Monday.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Samstag, 9. Dezember 2017 01:18
To: Langer, Christoph <[hidden email]>; [hidden email]
Cc: Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

All testing passed.

thanks,

Chris

On 12/8/17 1:07 PM, Langer, Christoph wrote:

Thanks in advance, Chris, for testing.

 

Please let me know the outcome when you are done.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Freitag, 8. Dezember 2017 21:52
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

On 12/8/17 5:48 AM, Langer, Christoph wrote:

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

If I understand correctly how the code currently works, pos/psPos will be NULL on the last iteration of the loop, but we'll exit anyway since "i == npaths" by that point. So the NULL pos will never be referenced. I guess coverity is cluing in on the following section:

 846         psPos = strchr(pos, ps[0]);
 847         if ( psPos == NULL ) {
 848             plen = (int)strlen(pos);
 849         } else {
 850             plen = (int)(psPos-pos);
 851             psPos++;
 852         }

We admit in line 847 that psPos can be NULL, but coverity can't read into the loop logic deep enough to recognize we'll also exit when this happens.

I guess your fix is fine, although technically unnecessary.




 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.



src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.

Looks good. Thanks for looking deeper into this one.


src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.

 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

Please list the files with just whitespace changes in the CR.


 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

I'm doing a pretty comprehensive round of serviceability testing myself with these changes and also 8193258. I'll let you know if there are any issues.

thanks,

Chris

 

Thanks in advance & Best regards

Christoph

 

 

 


Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Daniel D. Daugherty
Fixed.

Dan


On 12/11/17 2:53 AM, Chris Plummer wrote:
Hi Christoph,

I was just guilty of the same thing on Friday. Dan cleaned it up for me. See JDK-8191229. I believe the process is to change the Fix Version in the backport from 10 to 11, re-open it, and then close as resolved "No an issue" (with a comment as to why this was done). For the main CR, change the Fix Version from 11 to 10, copy the HG Updater info from the backport, close as resolved "Fixed". I think you also need to set "Resolved In Build" to "team".

Chris

On 12/10/17 11:39 PM, Langer, Christoph wrote:

Hi guys,

 

I just pushed this: http://hg.openjdk.java.net/jdk/jdk/rev/8db54e2c453b

 

However, for some reason somebody set my original bug to Fix version 11 which I kinda ignored and now the system created a backport bug for 10 (https://bugs.openjdk.java.net/browse/JDK-8193305). Probably I should have corrected the fix version before I pushed. Now, shall I set the original bug https://bugs.openjdk.java.net/browse/JDK-8192978 to “Resolved” manually or wait for the system to do it? Any hints on that one?

 

Thanks

Christoph

 

From: Langer, Christoph
Sent: Samstag, 9. Dezember 2017 18:08
To: 'Chris Plummer' [hidden email]; [hidden email]
Cc: '[hidden email]' [hidden email]
Subject: RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Chris,

 

thanks for testing. I added the files with whitespace changes to the bug. I’ll push this on Monday.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Samstag, 9. Dezember 2017 01:18
To: Langer, Christoph <[hidden email]>; [hidden email]
Cc: Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

All testing passed.

thanks,

Chris

On 12/8/17 1:07 PM, Langer, Christoph wrote:

Thanks in advance, Chris, for testing.

 

Please let me know the outcome when you are done.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Freitag, 8. Dezember 2017 21:52
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

On 12/8/17 5:48 AM, Langer, Christoph wrote:

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

If I understand correctly how the code currently works, pos/psPos will be NULL on the last iteration of the loop, but we'll exit anyway since "i == npaths" by that point. So the NULL pos will never be referenced. I guess coverity is cluing in on the following section:

 846         psPos = strchr(pos, ps[0]);
 847         if ( psPos == NULL ) {
 848             plen = (int)strlen(pos);
 849         } else {
 850             plen = (int)(psPos-pos);
 851             psPos++;
 852         }

We admit in line 847 that psPos can be NULL, but coverity can't read into the loop logic deep enough to recognize we'll also exit when this happens.

I guess your fix is fine, although technically unnecessary.




 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.



src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.

Looks good. Thanks for looking deeper into this one.


src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.

 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

Please list the files with just whitespace changes in the CR.


 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

I'm doing a pretty comprehensive round of serviceability testing myself with these changes and also 8193258. I'll let you know if there are any issues.

thanks,

Chris

 

Thanks in advance & Best regards

Christoph

 

 

 



Reply | Threaded
Open this post in threaded view
|

RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

Langer, Christoph

Hi Dan,

 

thanks for your help. I’ll be more wary next time…

 

Best regards

Christoph

 

From: Daniel D. Daugherty [mailto:[hidden email]]
Sent: Montag, 11. Dezember 2017 14:10
To: Chris Plummer <[hidden email]>; Langer, Christoph <[hidden email]>; [hidden email]; [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Fixed.

Dan

On 12/11/17 2:53 AM, Chris Plummer wrote:

Hi Christoph,

I was just guilty of the same thing on Friday. Dan cleaned it up for me. See JDK-8191229. I believe the process is to change the Fix Version in the backport from 10 to 11, re-open it, and then close as resolved "No an issue" (with a comment as to why this was done). For the main CR, change the Fix Version from 11 to 10, copy the HG Updater info from the backport, close as resolved "Fixed". I think you also need to set "Resolved In Build" to "team".

Chris

On 12/10/17 11:39 PM, Langer, Christoph wrote:

Hi guys,

 

I just pushed this: http://hg.openjdk.java.net/jdk/jdk/rev/8db54e2c453b

 

However, for some reason somebody set my original bug to Fix version 11 which I kinda ignored and now the system created a backport bug for 10 (https://bugs.openjdk.java.net/browse/JDK-8193305). Probably I should have corrected the fix version before I pushed. Now, shall I set the original bug https://bugs.openjdk.java.net/browse/JDK-8192978 to “Resolved” manually or wait for the system to do it? Any hints on that one?

 

Thanks

Christoph

 

From: Langer, Christoph
Sent: Samstag, 9. Dezember 2017 18:08
To: 'Chris Plummer' [hidden email]; [hidden email]
Cc: '[hidden email]' [hidden email]
Subject: RE: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Chris,

 

thanks for testing. I added the files with whitespace changes to the bug. I’ll push this on Monday.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Samstag, 9. Dezember 2017 01:18
To: Langer, Christoph <[hidden email]>; [hidden email]
Cc: Lindenmaier, Goetz <[hidden email]>
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

All testing passed.

thanks,

Chris

On 12/8/17 1:07 PM, Langer, Christoph wrote:

Thanks in advance, Chris, for testing.

 

Please let me know the outcome when you are done.

 

Best regards

Christoph

 

From: Chris Plummer [[hidden email]]
Sent: Freitag, 8. Dezember 2017 21:52
To: Langer, Christoph [hidden email]; [hidden email]
Cc: Lindenmaier, Goetz [hidden email]
Subject: Re: RFR(S): 8192978: Missing checks and small fixes in jdwp library

 

Hi Christoph,

On 12/8/17 5:48 AM, Langer, Christoph wrote:

Hi,

 

this is a new webrev for 8192978. This change now only contains the coverity fixes.

 

In detail:
src/jdk.jdwp.agent/share/native/libjdwp/VirtualMachineImpl.c, static void writePaths(PacketOutputStream *out, char *string):
    strchr could be called with NULL argument because of assignment pos = psPos; in line 856 (last line of for loop). Proposal: check pos for NULL in head of for loop

If I understand correctly how the code currently works, pos/psPos will be NULL on the last iteration of the loop, but we'll exit anyway since "i == npaths" by that point. So the NULL pos will never be referenced. I guess coverity is cluing in on the following section:

 846         psPos = strchr(pos, ps[0]);
 847         if ( psPos == NULL ) {
 848             plen = (int)strlen(pos);
 849         } else {
 850             plen = (int)(psPos-pos);
 851             psPos++;
 852         }

We admit in line 847 that psPos can be NULL, but coverity can't read into the loop logic deep enough to recognize we'll also exit when this happens.

I guess your fix is fine, although technically unnecessary.






 src/jdk.jdwp.agent/share/native/libjdwp/error_messages.c, static void vprint_message(FILE *fp, const char *prefix, const char *suffix, const char *format, va_list ap):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.





src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c, static jboolean synthesizeUnloadEvent(void *signatureVoid, void *envVoid):
    checking eventBag for NULL and then calling JDI_ASSERT only in that case is a bit dubious.

    It leads the coverity code scan tool to think that eventBag might be NULL when eventHelper_recordClassUnload is called

    which would eventually try to dereference a NULL eventbag and hence crash.

    Proposal: remove the NULL check but unconditionally assert.

 

    This is the real fix for the changes in eventHelper.c in my former webrev.

Looks good. Thanks for looking deeper into this one.



src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c, void log_message_end(const char *format, ...):
    potentially unterminated vsnprintf call. Proposal: terminate

Ok.


 

Furthermore I have 2 whitespace changes which I’d like to see because they would help me reducing the diff to our codebase and, furthermore, make the code looking nicer… a little ;-)

Please list the files with just whitespace changes in the CR.



 

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8192978.2/

Bug: https://bugs.openjdk.java.net/browse/JDK-8192978

 

I’m doing builds on the main platform and running jtreg tests.

I'm doing a pretty comprehensive round of serviceability testing myself with these changes and also 8193258. I'll let you know if there are any issues.

thanks,

Chris

 

Thanks in advance & Best regards

Christoph