Skip to content

Conversation

@jrtc27
Copy link
Contributor

@jrtc27 jrtc27 commented May 7, 2016

4e4d800 extended locks to have 64-bit keys in most cases, but some were left as 32-bit. However, code using these 32-bit locks assumed that the significant bytes of the key's long value started from lck_string[0], which is false on big-endian architectures. This commit adds Lock::getKeyString(), which gets a pointer to the first used byte of lck_string, and should be used in place of accessing lck_string directly.

4e4d800 extended locks to have 64-bit
keys in most cases, but some were left as 32-bit. However, code using
these 32-bit locks assumed that the significant bytes of the key's long
value started from lck_string[0], which is false on big-endian
architectures.  This commit adds Lock::getKeyString(), which gets a
pointer to the first used byte of lck_string, and should be used in
place of accessing lck_string directly.
@dyemanov dyemanov self-assigned this May 8, 2016
@dyemanov
Copy link
Member

dyemanov commented May 8, 2016

James, do you have an account at tracker.firebirdsql.org? I'd like to have this issue recorded there as well.

@dyemanov dyemanov merged commit 64fa4d3 into FirebirdSQL:master May 8, 2016
@jrtc27
Copy link
Contributor Author

jrtc27 commented May 8, 2016

Thanks; I just created an account and filed http://tracker.firebirdsql.org/browse/CORE-5232.

@dyemanov
Copy link
Member

dyemanov commented May 8, 2016

I will polish the getKeyString() function a little and then backport this bugfix into v3.0.1. Thanks for contribution.

@jrtc27
Copy link
Contributor Author

jrtc27 commented May 8, 2016

Great. The lck_length <= 8 check (I guess could be lck_length < 8, depends on which is more favourable for branch prediction) falling back on &lck_string[0] is very deliberate, as jrd_rel::getRelLockKeyLength() gives a 12-byte key. I missed that case originally and was returning &lck_string[8-lck_length] unconditionally (which, unsurprisingly, didn't work), hence this warning.

@jrtc27 jrtc27 deleted the lock-big-endian branch May 8, 2016 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants