You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by bu...@apache.org on 2012/02/14 09:50:50 UTC
DO NOT REPLY [Bug 52661] New: An incomplete fix for the NPE bug in
HyperlinkRecord.java
https://issues.apache.org/bugzilla/show_bug.cgi?id=52661
Bug #: 52661
Summary: An incomplete fix for the NPE bug in
HyperlinkRecord.java
Product: POI
Version: unspecified
Platform: PC
Status: NEW
Severity: critical
Priority: P2
Component: HSSF
AssignedTo: dev@poi.apache.org
ReportedBy: lianggt08@sei.pku.edu.cn
Classification: Unclassified
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);
}
}
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
DO NOT REPLY [Bug 52661] An incomplete fix for the NPE bug in
HyperlinkRecord.java
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=52661
lianggt08@sei.pku.edu.cn changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEEDINFO |NEW
--- Comment #2 from lianggt08@sei.pku.edu.cn 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.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
DO NOT REPLY [Bug 52661] An incomplete fix for the NPE bug in
HyperlinkRecord.java
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=52661
Yegor Kozlov <ye...@dinom.ru> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |NEEDINFO
--- Comment #3 from Yegor Kozlov <ye...@dinom.ru> 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.
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
DO NOT REPLY [Bug 52661] An incomplete fix for the NPE bug in
HyperlinkRecord.java
Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=52661
Nick Burch <ni...@alfresco.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |NEEDINFO
OS/Version| |All
--- Comment #1 from Nick Burch <ni...@alfresco.com> 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?
--
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org