[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size jhrozek commented: """ OK, good. There is a downstream request for a release that is based on the sssd-1-13 branch, so I'm quite sure I won't forget about the backport. btw it's not needed to run Coverity this week (unless you're bored), it's fine to do that when you do some other work in the stable branch. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-347345125 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size fidencio commented: """ I haven't ran coverity on sssd-1-13 branch. I'd say no. Let me run coverity at some point this week and then I can submit a PR with all the fixes. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-347334819 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size jhrozek commented: """ @fidencio do you know if the Coverity patch should be backported to sssd-1-13 as well? """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-347327875 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size lslebodn commented: """ http://vm-031.$ABC/logs/job/81/85/summary.html """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-347320388 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size lslebodn commented: """ master: * ec13304e0c4dc1ffe8c6abe6d0741fa7f6da3ceb * c4027058427d79d81404331ad0717907e56e87c4 """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-347319559 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size jhrozek commented: """ * master: * 1d88a0591ce8445ea3b6a88845a5997d61c915b4 * 4382047490dd4f80b407cc1e618da048f13e5f8f """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-347319569 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size lslebodn commented: """ master: * ec13304e0c4dc1ffe8c6abe6d0741fa7f6da3ceb * c4027058427d79d81404331ad0717907e56e87c4 """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-347319559 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size
lslebodn commented:
"""
On (27/11/17 14:23), sumit-bose wrote:
>I think it has to be at least
>
>- if (key->len > strs_len) {
>+ if (key->len + name_ptr - strs_offset > strs_len) {
>
>because name_ptr is relative to rec->data and includes some other uint32_t
>values the different record structs starts with which are not include in
>strs_len/data_len. But I think being more strict here is not needed because
>also for the passwd and group case strs_len includes other strings as well
>making the whole check only a rough estimate.
>
Probably yes, it was from top of my head.
>> IMHO it's faster then trying to find '\0'.
>
>I already removed the check in the latest version.
>
I noticed that and thank you.
I'm fine even with current version.
ACK++
LS
"""
See the full comment at
https://github.com/SSSD/sssd/pull/455#issuecomment-347198606
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size
sumit-bose commented:
"""
I think it has to be at least
- if (key->len > strs_len) {
+ if (key->len + name_ptr - strs_offset > strs_len) {
because name_ptr is relative to rec->data and includes some other uint32_t
values the different record structs starts with which are not include in
strs_len/data_len. But I think being more strict here is not needed because
also for the passwd and group case strs_len includes other strings as well
making the whole check only a rough estimate.
About
> IMHO it's faster then trying to find '\0'.
I already removed the check in the latest version.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/455#issuecomment-347196012
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size
lslebodn commented:
"""
On (24/11/17 15:42), sumit-bose wrote:
>I agree this is nitpicking and artificial, but I think strcmp() can run
>outside the data table.
>
>If the last slot in the data table is the last of an initgr record with many
>GIDs for a user with a short user name, e.g. 'user' and the slot is fully used
>so that the user name is at the end of the slot. data_len of in the initgr
>record (which is used for strs_len in sss_mc_find_record() in this case) will
>be large keys much longer then 'user' will pass until the strcmp. If now due
>to a corruption the '0' at the end of the user name (and the unique name) are
>replaced by a '-' with a key like 'user-user-x' strcmp() will read the first
>byte of the free table when trying to compare the 'x'. But I agree that even
>then it would not do any harm because we are still reading from our own memory.
>
Ahh that's true because GIDs are stored before strings for initgtoup
and strs_len (sss_mc_get_strs_len) is actually data_len for initgroups.
That case could be covered by stricter check for length of input string
```
-if (key->len > strs_len) {
+if (key->len + name_ptr > strs_len) {
/* The string cannot be in current record */
slot = sss_mc_next_slot_with_hash(rec, hash);
continue;
}
```
But such check should be probably after checking the value of `name_ptr`.
So probably inside "if" which would backup "corrupted" memory cache.
IMHO it's faster then trying to find '\0'.
LS
"""
See the full comment at
https://github.com/SSSD/sssd/pull/455#issuecomment-347185728
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size sumit-bose commented: """ latest version has memchr() removed. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346856048 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size sumit-bose commented: """ I agree this is nitpicking and artificial, but I think strcmp() can run outside the data table. If the last slot in the data table is the last of an initgr record with many GIDs for a user with a short user name, e.g. 'user' and the slot is fully used so that the user name is at the end of the slot. data_len of in the initgr record (which is used for strs_len in sss_mc_find_record() in this case) will be large keys much longer then 'user' will pass until the strcmp. If now due to a corruption the '0' at the end of the user name (and the unique name) are replaced by a '-' with a key like 'user-user-x' strcmp() will read the first byte of the free table when trying to compare the 'x'. But I agree that even then it would not do any harm because we are still reading from our own memory. So I will remove the memchr(). But maybe, since I'm planning to do some changes to the memory cache anyways, it would make sense to add the name length explicitly the data records. This would make the checks and comparison much easier and faster because strncmp() can be used. What do you think? """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346855219 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size lslebodn commented: """ On (24/11/17 07:10), sumit-bose wrote: >I kept the memchr() check mainly for the initgroups cache data. For passwd and >group the first element in the string/data area is the name and hence >'key->len > strs_len' makes sure the strcmp() will not read pass the current >object. For the initgroups data the name is 'somewhere' in the data area >because the data starts with a list of GIDs. So to avoid that strcmp() goes >pass the end of the data area with a long key in the case of a corruption the >"length" of t_key has to be determined. Using strnlen() would be possible as >well but imo memchr() is more clear here. > I'm sorry I cannot see any benefit for initgroups case. I just can see performance impact in 99.% cases with iterating over data twice (firstly with `memchr` secondly with `strcmp`) It is not 100% cases because we still assume that memory cache can be corrupted. As I wrote in previous comment all checks before `memchr` are sufficient to ensure that comparing data with NUL terminated input will not overflow area of data table. I'm fine with checking bounds because comparing numbers are fast. But I cannot see any benefit of checking data twice. Or did I miss something why directly using `strcmp` is unsafe? LS """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346824326 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size sumit-bose commented: """ In the latest version I included the 'key->len > strs_len' check and modified the check if strs_len points out of the data section to avoid an overrun. I kept the memchr() check mainly for the initgroups cache data. For passwd and group the first element in the string/data area is the name and hence 'key->len > strs_len' makes sure the strcmp() will not read pass the current object. For the initgroups data the name is 'somewhere' in the data area because the data starts with a list of GIDs. So to avoid that strcmp() goes pass the end of the data area with a long key in the case of a corruption the "length" of t_key has to be determined. Using strnlen() would be possible as well but imo memchr() is more clear here. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346757406 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size mzidek-rh commented: """ On 11/23/2017 12:50 PM, lslebodn wrote: > On (23/11/17 11:31), mzidek-rh wrote: > >mzidek-rh commented on this pull request. > >I do not think such optimisation is needed > > And I do not think that these checks for corrupted memory cache are needed > since 1.11.90. But I'm fine with having them. > > >because it will almost always be evaluated as false > > That statement is true also for other checks for "corrupted" mmap cache. Please avoid cherry picking my sentences in a way that changes the original meaning and also do not put them in inappropriate context. The statement I said is NOT true for the checks for corrupted mmap cache, because the whole statement was: "I do not think such optimisation is needed because it will almost always be evaluated as false and in case it is evaluated as true it will only save us from doing few more checks. " So my point was that we do some computation in order to avoid just a slightly bigger computation, but we will almost never avoid it anyway. And as we know, the checks for corrupted mmap cache were not an optimisation, so the statement is simply not applicable to that case unless you remove part of the statement and put it to a new context. But by doing that you are creating your own statement that has nothing to do with what I said. But as I said, I do not dislike the check becuase in my (very subjective) opinion it makes the code easier to read. Michal > > >and in case it is evaluated as true it will only save us from doing > few more checks. But maybe having this check makes this part of code > more readable (at least it looks that way to me)? So I am OK with it. > > > > Thank you for confirmation that you are fine with such check. > > LS > """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346605877 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size lslebodn commented: """ On (23/11/17 11:31), mzidek-rh wrote: >mzidek-rh commented on this pull request. >I do not think such optimisation is needed And I do not think that these checks for corrupted memory cache are needed since 1.11.90. But I'm fine with having them. >because it will almost always be evaluated as false That statement is true also for other checks for "corrupted" mmap cache. >and in case it is evaluated as true it will only save us from doing few more >checks. But maybe having this check makes this part of code more readable (at >least it looks that way to me)? So I am OK with it. > Thank you for confirmation that you are fine with such check. LS """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346598250 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size mzidek-rh commented: """ The CI results for @fidencio 's change arrived. ACK to the patchset. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346316533 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size jhrozek commented: """ I'm confused, this PR has still the accepted label, but I guess we're still waiting for the CI results before pushing, right? """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346132594 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size mzidek-rh commented: """ Fabiano's change LGTM. I resubmitted the patchset to the CI. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346100524 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size sumit-bose commented: """ Ok, the new version includes @fidencio's patch as well. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346097469 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size fidencio commented: """ I personally would just re-submit this PR with that patch atop, push this one and I can close that one. Does it work for you? """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346084002 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size sumit-bose commented: """ @fidencio, ok, better safe than sorry. How to proceed? Close this and merge #456? """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346083645 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size fidencio commented: """ And I totally agree it's a false positive. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346081751 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size fidencio commented: """ @sumit-bose, I did a new run and coverity did not detected this as a dead-code. :-/ """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346081639 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size sumit-bose commented: """ @fidencio, can you do a new Coverity run with your patch? I'm asking because I wonder if Coverity would be in the presence of your patch, clever enough to detect it as dead code, because as @mzidek-rh said due to earlier tests memchr() is not expected to return NULL here. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346080708 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size sumit-bose commented: """ @fidencio, can you do a nee Coverity run with your patch? I'm asking because I wonder if Coverity would be in the presence of your patch, clever enough to detect it as dead code, because as @mzidek-rh said due to earlier tests memchr() is not expected to return NULL here. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346080708 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size fidencio commented: """ @mzidek-rh, my patch does fix the warning and I do agree with you that it's just a false positive. Knowing that and agreeing with @jhrozek's comment, I'd like to have that patch used (if everyone agrees with that). """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346071740 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size jhrozek commented: """ I think a general rule of thumb is - if there is a simple way to make the code analyzers happy, then the code should be written so that even a machine understands it :-) If that's not possible or feasible, let's mark the coverity issue as a false positive. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346069476 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size mzidek-rh commented: """ Hi @fidencio , at first look I also thought covscan found an issue here, but looking closer this seems to be a false positive match. Few lines above the check that you added is already a check if the body of message is null terminated, so memchr here will never return NULL (it has to return at least pointer to the last '\0' in the message). """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346061933 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size
fidencio commented:
"""
I will **not** add the changes request label as the issue found is **not**
related to the code itself and could be fixed in a different commit. Do
@sumit-bose and @mzidek-rh agree with this?
A simple patch to (possibly) fix the issue is:
```
[ffidenci@pessoa x86_64]$ git diff
diff --git a/src/responder/nss/nss_protocol.c b/src/responder/nss/nss_protocol.c
index 265538649..f1cf30531 100644
--- a/src/responder/nss/nss_protocol.c
+++ b/src/responder/nss/nss_protocol.c
@@ -160,6 +160,10 @@ nss_protocol_parse_name_ex(struct cli_ctx *cli_ctx, const
char **_rawname,
}
p = memchr(body, '\0', blen);
+if (p == NULL) {
+DEBUG(SSSDBG_CRIT_FAILURE, "memchr() failed\n");
+return EINVAL;
+}
/* If the body isn't valid UTF-8, fail */
if (!sss_utf8_check(body, (p - body))) {
```
"""
See the full comment at
https://github.com/SSSD/sssd/pull/455#issuecomment-346053217
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size
fidencio commented:
"""
Sorry for jumping in a patch set which I'm neither the author nor the reviewer.
Covscan has found an issue with this patchset:
```
Error: NULL_RETURNS (CWE-476): [#def1]
sssd-1.16.1/src/responder/nss/nss_protocol.c:162: returned_null: "memchr"
returns null (checked 7 out of 8 times).
sssd-1.16.1/src/responder/nss/nsssrv_mmap_cache.c:557: example_checked: Example
1: "memchr(t_key, 0, strs_offset + strs_len - name_ptr)" has its value checked
in "memchr(t_key, 0, strs_offset + strs_len - name_ptr) == NULL".
sssd-1.16.1/src/sss_client/idmap/sss_nss_idmap.c:171: example_assign: Example
2: Assigning: "p" = return value from "memchr(p, 0, buf_len - (p - buf))".
sssd-1.16.1/src/sss_client/idmap/sss_nss_idmap.c:172: example_checked: Example
2 (cont.): "p" has its value checked in "p == NULL".
sssd-1.16.1/src/sss_client/nss_mc_group.c:157: example_checked: Example 3:
"memchr(rec_name, 0, 16UL + data->strs_len - data->name)" has its value checked
in "memchr(rec_name, 0, 16UL + data->strs_len - data->name) == NULL".
sssd-1.16.1/src/sss_client/nss_mc_initgr.c:139: example_checked: Example 4:
"memchr(rec_name, 0, 24UL + data->data_len - data->name)" has its value checked
in "memchr(rec_name, 0, 24UL + data->data_len - data->name) == NULL".
sssd-1.16.1/src/sss_client/nss_mc_passwd.c:150: example_checked: Example 5:
"memchr(rec_name, 0, 16UL + data->strs_len - data->name)" has its value checked
in "memchr(rec_name, 0, 16UL + data->strs_len - data->name) == NULL".
sssd-1.16.1/src/responder/nss/nss_protocol.c:162: var_assigned: Assigning: "p"
= null return value from "memchr".
sssd-1.16.1/src/responder/nss/nss_protocol.c:176: dereference: Incrementing a
pointer which might be null: "p".
# 174| }
# 175|
# 176|-> p++;
# 177| if ((p - body) + sizeof(uint32_t) != blen) {
# 178| DEBUG(SSSDBG_CRIT_FAILURE, "Body has unexpected size!\n");
```
"""
See the full comment at
https://github.com/SSSD/sssd/pull/455#issuecomment-346051695
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size mzidek-rh commented: """ Ack. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-346023383 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size mzidek-rh commented: """ Thanks Sumit. Just waiting for the CI to finish before ACKing. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-345974016 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size sumit-bose commented: """ Thank you for the review, I included your changes in the latest version. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-345956489 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455
Title: #455: mmap_cache: make checks independent of input size
mzidek-rh commented:
"""
Just two really minor nitpicks (typo in word 'relative' and missing space
around '+'):
```
diff --git a/src/responder/nss/nsssrv_mmap_cache.c
b/src/responder/nss/nsssrv_mmap_cache.c
index 5c87484..3441c79 100644
--- a/src/responder/nss/nsssrv_mmap_cache.c
+++ b/src/responder/nss/nsssrv_mmap_cache.c
@@ -550,7 +550,7 @@ static struct sss_mc_rec *sss_mc_find_record(struct
sss_mc_ctx *mcc,
safealign_memcpy(_ptr, rec->data, sizeof(rel_ptr_t), NULL);
t_key = (char *)rec->data + name_ptr;
/* name_ptr must point to some data in the strs/gids area of the data
- * payload. Since it is a pointer realtive to rec->data it must larger
+ * payload. Since it is a pointer relative to rec->data it must larger
* equal strs_offset and must be smaller then strs_offset + strs_len.
* Additionally the area must not end outside of the data table and
* t_key must be a zero-terminates string. */
diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
index dd925a4..4b1601a 100644
--- a/src/sss_client/nss_mc_group.c
+++ b/src/sss_client/nss_mc_group.c
@@ -157,7 +157,7 @@ errno_t sss_nss_mc_getgrnam(const char *name, size_t
name_len,
if (data->name < strs_offset
|| data->name >= strs_offset + data->strs_len
|| data->strs_len > rec->len
-|| (uint8_t *) rec+ rec->len > gr_mc_ctx.data_table + data_size
+|| (uint8_t *) rec + rec->len > gr_mc_ctx.data_table + data_size
|| memchr(rec_name, '\0',
(strs_offset + data->strs_len) - data->name) == NULL) {
ret = ENOENT;
```
"""
See the full comment at
https://github.com/SSSD/sssd/pull/455#issuecomment-345783134
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#455][comment] mmap_cache: make checks independent of input size
URL: https://github.com/SSSD/sssd/pull/455 Title: #455: mmap_cache: make checks independent of input size jhrozek commented: """ Since @mzidek-rh was already looking into the issue earlier, I hope Michal can help with the review as well. """ See the full comment at https://github.com/SSSD/sssd/pull/455#issuecomment-345282431 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
