Crypt32Util.cryptProtectData fails on zero-length array

[dmytro-sheyko]

Allocated 1-byte array in case when 0-length array is supplied in order to avoid IllegalArgumentException
Generally speaking LocalAlloc accepts size=0, by malloc (which is used in this place) may return null in this case, which is indistinguishable from OOM condition.

[matthiasblaesing]

A 1 byte array is not zero length byte array. The result will be broken. Please have a look at the discussion about the issue. I placed a suggestion there.

[dbwiddis]

If I read this comment correctly, you're saying a null pointer is invalid, so we need a valid pointer that we will ignore the contents of. If this is the case, the subsequent pbData.write() call is useless. I think rather than always invoking Math.max() this should be a simple conditional. If data.length > 0 we create memory of that length and write to it; otherwise we can create (or use) any valid pointer allocation.

[dbwiddis]

A 1 byte array is not zero length byte array. The result will be broken. Please have a look at the discussion about the issue. I placed a suggestion there.

Per the comment, the suggestion uses null which Windows doesn't like. It needs a valid pointer to "owned" memory, but the length is specified so over-allocating is not a problem.

[dbwiddis]

Looking further at this constructor, it's odd there's an allocateMemory() call in it. DATA_BLOB is simply a fixed-size structure with an int and a Pointer.

Also if you're not reading data from a pointer, any pointer will do and we already have a pointer for the DATA_BLOB structure itself. So I think this would work:

        public DATA_BLOB(byte[] data) {
            super();
            if (data.length > 0) {
                pbData = new Memory(data.length);
                pbData.write(0, data, 0, data.length);
            } else {
                // For zero length array we just need a pointer to any allocated memory, the
                // data there will be ignored
                pbData = getPointer();
            }
        }

[dbwiddis]

You need a call to super() before this.

[dmytro-sheyko]

no problem. is it to conform code convention?

[dbwiddis]

It's needed to allocate the memory for the structure, among other things. We removed that memory allocation at the end, which seemed to imply we didn't have all the information needed until after we'd populated the pointer -- but we did, it's a fixed size array.

[dbwiddis]

But with my suggested solution the getPointer() would return null until it was allocated.

[dmytro-sheyko]

I thought that javac adds super() call automatically.
https://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.8.7

If a constructor body does not begin with an explicit constructor invocation and the constructor being declared is not part of the primordial class Object, then the constructor body implicitly begins with a superclass constructor invocation "super();", an invocation of the constructor of its direct superclass that takes no arguments.

[dbwiddis]

OK, you're absolutely correct. This is one of those cases where I've done it so much I forgot that it was just a "good idea". So yes, I suppose it boils down to style, but here are some good reasons I choose to always do so here:

• Consistency. All the other constructors in this class have it (including the no-arg one). This is common in JNA, where if one constructor declares it the no-arg one is almost always included as well (often the pair super() on the no-arg and super(p); read(); on the pointer constructor) even if technically optional. Omitting it looks like an oversight because it's "different".
• Clarity. Calling a no-arg superconstructor from a constructor with an argument is not common, and this makes it more clear that it's an explicit choice and not an oversight.
• IDE navigation. I often find myself double-checking just exactly what the Structure constructors do and it's nice to navigate directly to the correct line from within my IDE.
• Error-checking. Ever forgotten extends on the class when mapping JNA structures? I have, more than once. (Don't laugh.). This helps catch that silly error early.

There may be other reasons for or against, particularly in the more general case, but for this particular class i think the "consistency" argument above is reason enough.

[matthiasblaesing]

This will become problematic when you call Pointer#clear on pbData. If then Structure#read is invoked on the DATA_BLOB structure, the content changes, although nothing "did" that. While I don't see an immediate problem, I learned, that side effects come back to haunt us.

I think the Memory(1) solution is ugly, but i don't see an obvious performance problem. If the hot path involves creating DATA_BLOBs from empty arrays, the issue is somewhere else.

[dbwiddis]

Fair enough, although the clearing specifies 0 length so it shouldn't impact. Still, a fresh new pointer avoids any potential issues.

[dmytro-sheyko]

If you believe that Memory(1) is ugly, how about to allocate memory by LocalAlloc instead of malloc? And then free it explicitly within CryptProtectData and CryptUnprotectData using LocalFree (as we do with the result). LocalAlloc behaves nicely with size=0 in contrast to malloc.
But in my opinion, solution with Memory(1) is quite safe and reliable and assumes minimal code changes. However it appeared it isn't very obvious.

[matthiasblaesing]

I think Memory(1) is the best solution here (together with a comment why a 1 byte memory area is allocated, although a 0 lenght byte array is supplied). Ugly in this case means, that I think MS should not rely on definition hole in the C specification.

[dbwiddis]

This is not a suggestion to actually do this, but I did go down the road of thinking about new Pointer(Native.malloc(0)); (and eventually freeing it). Just to show there's an uglier solution than Memory(1). :)

[matthiasblaesing]

If you add an entry to the CHANGES.md this looks good to merge.

[matthiasblaesing]

Looks sane to me - thank you.