Bug 52661 - An incomplete fix for the NPE bug in HyperlinkRecord.java
Summary: An incomplete fix for the NPE bug in HyperlinkRecord.java
Status: RESOLVED WORKSFORME
Alias: None
Product: POI
Classification: Unclassified
Component: HSSF (show other bugs)
Version: unspecified
Hardware: PC All
: P2 critical (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-14 08:50 UTC by lianggt08
Modified: 2015-05-31 22:18 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description lianggt08 2012-02-14 08:50:50 UTC
The fix revision 720997 was aimed to remove an NPE bug on the "this._moniker" in the method "toString" of the file "/poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java" , but it is incomplete. 
Since the "this._moniker" is a class field and also could be null during the run-time execution, it should also be null-checked before being dereferenced in other methods. 

The buggy code locations the same fix needs to be applied at are as bellows: 
Line 559 of the method "serialize". 


	public void serialize(LittleEndianOutput out) {
	        _range.serialize(out);
	
	        _guid.serialize(out);
	        out.writeInt(0x00000002); // TODO const
	        out.writeInt(_linkOpts);
	
	        if ((_linkOpts & HLINK_LABEL) != 0){
	            out.writeInt(_label.length());
	            StringUtil.putUnicodeLE(_label, out);
	        }
	        if ((_linkOpts & HLINK_TARGET_FRAME) != 0){
	            out.writeInt(_targetFrame.length());
	            StringUtil.putUnicodeLE(_targetFrame, out);
	        }
	
	        if ((_linkOpts & HLINK_UNC_PATH) != 0) {
	            out.writeInt(_address.length());
	            StringUtil.putUnicodeLE(_address, out);
	        } else if ((_linkOpts & HLINK_URL) != 0){
[Line 559]	            _moniker.serialize(out);
	            if(URL_MONIKER.equals(_moniker)){
	                if ((_linkOpts & HLINK_TARGET_FRAME) != 0) {
	                    out.writeInt(_address.length()*2);
	                    StringUtil.putUnicodeLE(_address, out);
	                } else {
	                    out.writeInt(_address.length()*2 + TAIL_SIZE);
	                    StringUtil.putUnicodeLE(_address, out);
	                    writeTail(_uninterpretedTail, out);
	                }
	            } else if (FILE_MONIKER.equals(_moniker)){
	                out.writeShort(_fileOpts);
	                out.writeInt(_shortFilename.length());
	                StringUtil.putCompressedUnicode(_shortFilename, out);
	                writeTail(_uninterpretedTail, out);
	                if (_address == null) {
	                    out.writeInt(0);
	                } else {
	                    int addrLen = _address.length() * 2;
	                    out.writeInt(addrLen + 6);
	                    out.writeInt(addrLen);
	                    out.writeShort(0x0003); // TODO const
	                    StringUtil.putUnicodeLE(_address, out);
	                }
	            }
	        }
	        if((_linkOpts & HLINK_PLACE) != 0){
	               out.writeInt(_textMark.length());
	            StringUtil.putUnicodeLE(_textMark, out);
	        }
	    }
Comment 1 Nick Burch 2012-02-14 12:52:42 UTC
I believe that a monkier is required for both of the two kinds of links that are being serialized there. If the monkier is null at this point, then the problem lies earlier. We can't just skip writing it, as the record wouldn't be valid

How are you ending up without one?
Comment 2 lianggt08 2012-02-15 06:12:10 UTC
The text diff of the revision 720997 is as bellows: 
--- poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java	2008/11/26 22:00:07	720996
+++ poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java	2008/11/26 22:00:29	720997
@@ -628,7 +628,7 @@
         if ((_linkOpts & HLINK_TARGET_FRAME) != 0) {
             buffer.append("    .targetFrame= ").append(getTargetFrame()).append("\n");
         }
-        if((_linkOpts & HLINK_URL) != 0) {
+        if((_linkOpts & HLINK_URL) != 0 && _moniker != null) {
             buffer.append("    .moniker   = ").append(_moniker.formatAsString()).append("\n");
         }
         if ((_linkOpts & HLINK_PLACE) != 0) {

we can see that only checking "(_linkOpts & HLINK_URL) != 0" can not guarantee
that " _moniker != null". 

If the revision 720997 is a right fix, then I think the Line 559 of the method "serialize" should also be fixed this way. 

I think it will be more reasonable to treat them in a same way.
Comment 3 Yegor Kozlov 2012-02-15 08:39:30 UTC
I would say that toString() must correspond to serialize(), not the other way around.

The code in HyperlinkRecord.serialize is correct. If a link is URL and not a UNC path then the moniker field MUST be set. If it is null then you are constructing wrong data.

HyperlinkRecord.toString() uses slightly different logic and prints the moniker if it is not null and hyperlink type is URL (UNC path option is not tested here). This is not quite correct but acceptable because toString() is used mainly for debugging purposes. 

In any case, I'm not going to change it without a unit test that demonstrates the NPE issue. If you are able to construct one, please upload and re-open this ticket.

Yegor 

(In reply to comment #2)
> The text diff of the revision 720997 is as bellows: 
> --- poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java   
> 2008/11/26 22:00:07    720996
> +++ poi/trunk/src/java/org/apache/poi/hssf/record/HyperlinkRecord.java   
> 2008/11/26 22:00:29    720997
> @@ -628,7 +628,7 @@
>          if ((_linkOpts & HLINK_TARGET_FRAME) != 0) {
>              buffer.append("    .targetFrame=
> ").append(getTargetFrame()).append("\n");
>          }
> -        if((_linkOpts & HLINK_URL) != 0) {
> +        if((_linkOpts & HLINK_URL) != 0 && _moniker != null) {
>              buffer.append("    .moniker   =
> ").append(_moniker.formatAsString()).append("\n");
>          }
>          if ((_linkOpts & HLINK_PLACE) != 0) {
> 
> we can see that only checking "(_linkOpts & HLINK_URL) != 0" can not guarantee
> that " _moniker != null". 
> 
> If the revision 720997 is a right fix, then I think the Line 559 of the method
> "serialize" should also be fixed this way. 
> 
> I think it will be more reasonable to treat them in a same way.
Comment 4 Dominik Stadler 2015-05-31 22:18:49 UTC
Closing based on previous comment and non-update for a long time.