HPCC-10323 ESP incorrectly checks for errors querying file perms #5080
Conversation
In dasess.cpp, CClientSessionManager::getPermissionsLDAP never reads the error code from the message buffer into the local 'e' variable. This code fix reads the error code, ensuring either the caller will receive the error or an exception will be thrown if the caller does not want it Signed-off-by: William Whitehead <william.whitehead@lexisnexis.com>
|
Jira updated |
|
@jakesmith Please review |
| int e = 0; | ||
| if (mb.remaining()>=sizeof(e)) { | ||
| if (mb.remaining()>=sizeof(int)) { | ||
| int e = 0; |
There was a problem hiding this comment.
Looks ok, but now the above few lines of code do both sizeof(int) and sizeof().
Should be consistent, I think better as it was (sizeof(e))
There was a problem hiding this comment.
It was changed to sizeof(int) because e is not in scope , I assume
There was a problem hiding this comment.
Yes, but either the 'ret' case should look the same, i.e. sizeof(int) and int ret moved into block, or it should be sizeof(e) and int e moved out of block again. I prefer the sizeof(variable name) approach, but not fussed really as long as consistent.
There was a problem hiding this comment.
Moving 'int ret" is beyond the scope of this bug fix (as I have been told before) so I stuck to fixing the problem at hand. It doesn't make sense to allocate stack space for "int e" if there is nothing to be read into it, hence my modification to move it within the "if (mb.remaining()>=sizeof(int)) {" block
There was a problem hiding this comment.
This is not time critical, so allocating stack for int e, is not going to matter.
The main point is the two different approaches within a few lines code.
There was a problem hiding this comment.
I think Jake is being a bit over-critical.
There was a problem hiding this comment.
Well, have to find something to comment on..
Though still maintain makes for more readable code if consistent
HPCC-10323 ESP incorrectly checks for errors querying file perms Reviewed-By: Jake Smith <jake.smith@lexisnexis.com> Reviewed-By: Richard Chapman <rchapman@hpccsystems.com>
In dasess.cpp, CClientSessionManager::getPermissionsLDAP never reads
the error code from the message buffer into the local 'e' variable. This
code fix reads the error code, ensuring either the caller will receive
the error or an exception will be thrown if the caller does not want it
Signed-off-by: William Whitehead william.whitehead@lexisnexis.com