BLOBFileValue() might be discarded to early

• Key: JCR-428
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Stefan Guggisberg
• Reporter: Michael Frericks
• Component/s: jackrabbit-core
• Affects Version/s: 1.0
• Fix Version/s: 1.0.1

Description

Situation:

if the internal value of a property of type binary is created by the constructor BLOBFileValue(InputStream in) and the content is not stored in an temp-file, then calling the methods

a) #setProperty(InputStream in) on this node and then
b) #refresh(false) on the node of this property

on the node of this property leads to an internal value of this property with an erased byte[].

Solution:

Only if the spoolFile is created the field 'temp' should be set to true.
If the InputStream is stored in the byte[] the field 'temp' should be set to false.

Patch:

Index: BLOBFileValue.java
===================================================================
retrieving revision 1.1
diff -u -r1.1 BLOBFileValue.java
--- BLOBFileValue.java 8 May 2006 13:57:49 -0000 1.1
+++ BLOBFileValue.java 8 May 2006 15:19:54 -0000
@@ -142,6 +142,7 @@
                     len += read;
                 }
             }
+ in.close();
         } finally {
             if (out != null) {
                 out.close();
@@ -151,8 +152,15 @@
         // init vars
         file = spoolFile;
         fsResource = null;
- // this instance is backed by a temporarily allocated resource/buffer
- temp = true;
+ if (file != null)
+ {
+ // this instance is backed by a temporarily allocated resource
+ temp = true;
+ }
+ else
+ {
+ temp = true;
+ }
     }
 
     /**

Comments

[Michael Frericks]

Sorry, patch should be:

Index: BLOBFileValue.java
===================================================================
retrieving revision 1.1
diff -u -r1.1 BLOBFileValue.java
--- BLOBFileValue.java 8 May 2006 13:57:49 -0000 1.1
+++ BLOBFileValue.java 8 May 2006 15:19:54 -0000
@@ -142,6 +142,7 @@
                     len += read;
                 }
             }
+ in.close();
         } finally {
             if (out != null) {
                 out.close();
@@ -151,8 +152,15 @@
         // init vars
         file = spoolFile;
         fsResource = null;
- // this instance is backed by a temporarily allocated resource/buffer
- temp = true;
+ if (file != null)
+ {
+ // this instance is backed by a temporarily allocated resource
+ temp = true;
+ }
+ else
+ {
+ temp = false;
+ }
     }
 
     /**

[Stefan Guggisberg]

the current implementation is IMO correct.

the 'temp' flag is set to true because either a temp file has been created *or* a
buffer has been dynamically allocated, which the in-line comment correclty states:

        // this instance is backed by a temporarily allocated resource/buffer
        temp = true;

on #discard the buffer is correctly disposed.

please note that BLOBFileValue is for Jackrabbit-internal use only:
http://jackrabbit.apache.org/api-1/org/apache/jackrabbit/core/value/BLOBFileValue.html

[Michael Frericks]

I think the bug ist that BLOBFileValue is reused after discard ist called. Discard erases the field buffer or the file. At the point of resue this leads to a empty property not matching the correct value of the property.

And this is what i observed using the org.apache.jackrabbit.core.state.orm.ojb.OJBPersistenceManager.

Inside its method #load(PropertyId propId) this class calls

InternalValue create(InputStream value)-> public BLOBFileValue(InputStream in)

which is the only constructor of BLOBFileValue that sets "temp = true".

[Stefan Guggisberg]

> I think the bug ist that BLOBFileValue is reused after discard ist called.
> Discard erases the field buffer or the file. At the point of resue this leads
> to a empty property not matching the correct value of the property.

where in jackrabbit's code are BLOBFileValue instances reused after they
have been discarded?

what's the exact issue here? are there any jcr api calls that fail?

please provide a simple test case that, using the default configuration,
i.e. DerbyPersistenceManager, demonstrates the issue.

[Przemo Pakulski]

I think this can be related also of JCR-262 (restore sometime throws error about missing tmp files).

We are getting following exceptions often:

javax.jcr.RepositoryException: file backing binary value not found: C:\tomcat\temp\bin63780.tmp (The system cannot find the file specified): C:\tomcat\temp\bin63780.tmp (The system cannot find the file specified)
    at com.day.crx.core.value.BLOBFileValue.getStream(BLOBFileValue.java:457)

I can't reproduce this yet and I don't know where BLOBFileValue instances reused after discard but it looks it is (maybe cache?).

[Tobias Bocanegra]

the problem is, that in the first place, BLOBFileValues were not thought of beeing produced by the backend, but rather via the client, from the transient side. the late introduction of db-stored blobs just reused them without checking all invariants.

the following example shows this error (when having db-blobs):

            Node rootNode = session.getRootNode();

            Node blobNode = rootNode.addNode("blobNode");
            rootNode.save();

            blobNode.setProperty("data", BlobTest3.createStream(70000));
            blobNode.save();

            blobNode.setProperty("data", BlobTest3.createStream(70000));
            blobNode.refresh(false);

            blobNode.getProperty("data").getString();

and throws an exception on the last .getString() since the BLOBFileValue gets discarded in the 2nd setProperty() call.

i think in the long run, we need a proper Blob/BlobFactory/reference counting framework in place, in order to properly support both client and backend usage of those blob file values. that's why i also opt for having a new value type: Binary in the jcr283 spec.

in the meantime, i think it helps to make the blobvalues of the transient items non-temporary, so that they don't get discarded.

[Tobias Bocanegra]

fixed.

Committed revision 407709.

[Stefan Guggisberg]

ok, michael was right. in certain scenarios a BLOBFileValue instance could indeed
get accidentaly discarded because of jackrabbit's copy-on-write implementation
which does a shallow copy of the value(s) for efficiency and performance reasons.

a blob read from the db was wrongly considered being 'transient' because it was
internally created from a stream. if a copy thereof would be lateron discarded the
underlying value would be discarded as well (because of the shallow copy-on-write).

i added a specialized constructor for creating BLOBFileValue instances from a stream
which won't get discarded:

    public BLOBFileValue(InputStream in, boolean temp) throws IOException

fixed in svn r408647

[Jukka Zitting]

Merged for 1.0.1 in revision 409535.