Add Cfgmgr32 functions for navigating device tree

[dbwiddis]

No description provided.

[matthiasblaesing]

In general this looks good. I have a small nitpick - see separate comment.

[matthiasblaesing]

Please check the return of CM_Get_Device_ID. The call sequence is correct, but the identifier could increase in length between the call to CM_Get_Device_ID_Size and here. Then either a second call can be tried or an error be raised.

[dbwiddis]

Interestingly, the function returns success in the case where there's enough room for the characters but not the null terminator. I've tried one way of handling it (incrementing until successful). We could also keep re-measuring the length to try to include the null. Let me know which you'd prefer.

[matthiasblaesing]

I had another look at Cfgmgr32Util#CM_Get_Device_ID. Reading strings without NULL terminators is dangerous, as it could not only read "random" characters, but could cause a segmentation fault. The change works by acknowledging that the function does not always write a NULL terminator, so:

• a memory block is allocated, that is one character larger than reported by CM_Get_Device_ID_Size
• the memory block is zeroed
• CM_Get_Device_ID is invoked with the original size, so won't write a NULL terminator (and if it does, no problem), in any case we have a closing NULL (the memory CM_Get_Device_ID won't write to).

Could you please have a look at the attached patch?

patch.txt

[dbwiddis]

I don't think your patch fixes the problem. Other than capping the number of retries, you do the same thing: get id size, add one, fetch the id. This will work 99.9999% of the time. Clearing the buffer doesn't make a difference.

The edge case occurs if both:

• the DeviceID length increases between CM_Get_Device_ID_Size and CM_Get_Device_ID.
• the increase is exactly one character

The problem isn't adding the character -- we could allocate 4096 bytes and almost certainly ensure we have enough length (and a null terminator). But we still wouldn't be sure. The problem is that we can't trust the return code to guarantee the presence of a null. Either the string is less than the allocated length (and there's a null) or the string is exactly the allocated length (and there's no null).

If you specify the exact length of the string, CM_Get_Device_ID method still returns CR_SUCCESS. The buffer is filled with the string (as expected) but no null terminator is added (it would be outside the allocated buffer and would be touching unallocated memory. The Pointer.getString() or Pointer.getWideString() methods don't know where to stop.

I think one workaround is to not use Pointer.getString() but instead get a specified length char[] (or byte[] if ascii), iterate these arrays looking for the null (or ending at max length), and converting them to Strings of the specified length.

Another workaround would be to take the returned buffer, allocate a new buffer 1 character larger (and clear it or just write 0 to the last character), copy the original buffer over byte-by-byte, and then use getString() on the second buffer, which is now guaranteed to have a null.

[dbwiddis]

OK, I gave it another shot. Fundamental differences:

• Don't bother to add 1 to buffer (to fit the null) if we can't rely on it
• Only retry once. This is an already rare enough event (deviceId change) competing with a race condition (change between measuring size and fetching the string).
• Process the returned buffer as a String.

[matthiasblaesing]

To give some more context to my suggestion - the important part of my suggestion was this:

• allocate memory for X + 1 characters
• zero the memory, now you have X+1 NULL terminators
• tell CM_Get_Device_ID that the size of the buffer is X characters

The last index CM_Get_Device_ID may write to is X-1, else it would write outside the memory it is allowed to write to. Index X will always be NULL and work as the string terminator. If the string gets shorter, an earlier NULL terminator will be written by the function.

For your current implementation: Hardcoding US-ASCII is a bad idea. Most probably the IDs only contain ASCII, but I would not rely on it, please have a look at Native#toString that is overloaded for byte[] and char[], the call to trim is unnecessary then.

[dbwiddis]

Ah, I missed the subtlety there where the length you were passing to CM_Get_Device_ID was one character shorter than the buffer actually was.

I think my latest patch matches your suggestion except I only did one retry. Unless you think my implementation with better byte[] and char[] treatment is preferred to the buffer shenanigans (I think it actually would be more readable that way.)

[dbwiddis]

Actually, after rereading the docs, I think the "shenanigans" version I have currently committed is better, as the docs specifically mention adding room for the null.

[matthiasblaesing]

Thanks - I agree with your change. I added two final comments, please have a look at them.

[matthiasblaesing]

Could you please add a return value check here - if this is not CR_SUCCESS throw an Cfgmgr32Exception exception with the value.

[matthiasblaesing]

Please add throws Cfgmgr32Exception to the signature. It does not need to be there, but I think it is clearer, that the caller should be prepared for the exception.

[dbwiddis]

Should be good to go now. Thanks for your patience with an amateur coder, your code reviews are very instructive! One of these days I'll get a PR through without modifications. :)

[matthiasblaesing]

Thank you for your effort. This looks good. I'll merge it.