[SSSD] [PATCH] TESTS: fix segfault in test_sss_strerror_err_last

Sumit Bose sbose at redhat.com
Fri May 29 10:45:26 UTC 2015


On Fri, May 29, 2015 at 12:08:27PM +0200, Lukas Slebodnik wrote:
> On (29/05/15 12:02), Sumit Bose wrote:
> >On Fri, May 29, 2015 at 11:17:01AM +0200, Pavel Reichl wrote:
> >> 
> >> 
> >> On 05/28/2015 06:52 PM, Sumit Bose wrote:
> >> >On Thu, May 28, 2015 at 06:26:43PM +0200, Pavel Reichl wrote:
> >> >>
> >> >>On 05/28/2015 06:12 PM, Sumit Bose wrote:
> >> >>>On Thu, May 28, 2015 at 05:57:35PM +0200, Lukas Slebodnik wrote:
> >> >>>>On (28/05/15 17:53), Pavel Reichl wrote:
> >> >>>>>On 05/28/2015 05:45 PM, Lukas Slebodnik wrote:
> >> >>>>>>On (28/05/15 17:25), Pavel Reichl wrote:
> >> >>>>>>>Hello,
> >> >>>>>>>
> >> >>>>>>>please see simple attached patch. Although test catches the bug and is no
> >> >>>>>>>doubt useful I believe that we should avoid segfaulting even in tests.
> >> >>>>>>>
> >> >>>>>>>Thanks for considering!
> >> >>>>>>>From a509c49fbc55664170d96e0e29bf6263c8d38a2a Mon Sep 17 00:00:00 2001
> >> >>>>>>>From: Pavel Reichl <preichl at redhat.com>
> >> >>>>>>>Date: Thu, 28 May 2015 11:13:47 -0400
> >> >>>>>>>Subject: [PATCH] TESTS: fix segfault in test_sss_strerror_err_last
> >> >>>>>>>
> >> >>>>>>>If there were more error codes than error messages. This
> >> >>>>>>>test segfaulted.
> >> >>>>>>>
> >> >>>>>>When test crashed then it failed.
> >> >>>>>I don't think that relying on undefined behavior is a good practice.
> >> >>>>It's not undefined behaviour.
> >> >>>>becuase you should not dereference NULL pointer.
> >> >>>>
> >> >>>>>>So the problem was not in test but in sssd code.
> >> >>>>>Well technically the test is part of SSSD code suite IMO.
> >> >>>>Test can crash if the bug is in the code.
> >> >>>>result of make check is non zero -> failed.
> >> >>>I agree with Lukas here. The crash is not in the test. but in the code.
> >> >>>If I see it correctly from you patch sss_strerror() crashes if the error
> >> >>>code is valid but there is no matching error message, because the array
> >> >>>is too short. Instead of doing the check in the test the check should be
> >> >>>done in sss_strerror() and if there is a mismatch a special error
> >> >>>should be return. The test then checks for proper behaviour and fails id
> >> >>>the special error is returned indication a mismatch in error codes and
> >> >>>error messages.
> >> >>>
> >> >>>HTH
> >> >>>
> >> >>>bye,
> >> >>>Sumit
> >> >>Sumit, thanks for comment. But I'm little confused here, do you agree with
> >> >>Lukas that this test is implemented just fine or do you agree with me that
> >> >I think the test is implemented fine, it checks if the last error
> >> >message is the expected message. If the test crashes, it is not the
> >> >fault of the test but the tested call, sss_strerror() has an issue.
> >> Thanks for explaining. I'll probably send the patch on sss_strerror() then.
> >> >
> >> >>segfault is not good test output?
> >> >I think it is neither good or bad, it just indicates that the test
> >> >discovered an error.
> >> >
> >> >bye,
> >> >Sumit
> >> OK, apparently I'm on the wrong side of this discussion. I yield, I just
> >> want to learn from my mistakes, so can you please tell me when I was wrong:
> >> 
> >> a) Dereferencing NULL pointer is from C language pov undefined behavior and
> >> virtually anything can happen => test might pass.
> >> b) It's not good practice to expect linux specific ability to handle
> >> dereferencing NULL pointer by segfaulting.
> >> c) Considering test that does only one thing - comparing number of error
> >> messages to error codes to be in need for improvement because it's not
> >> deterministic (a).
> >
> >If there is something wrong at all, I would say is it in c). First the
> >current test
> >
> >ck_assert_str_eq(sss_strerror(ERR_LAST), "ERR_LAST");
> >
> >does in fact two things. First it calls sss_strerror() with a valid
> >input (any int is a valid input here), second it compares the returned
> >value with the expected one. The test is completely deterministic
> >because only if the returned value matches the expected value the test
> >passed. In all other cases (including segfaults) the test must be
> >considered failed because it is expected that sss_strerror() always
> >returns a string and never NULL. 
> >
> >The segfault is caused by calling sss_strerror() with ERR_LAST as
> >argument.
> I would say segfault was caused by fact that someone add new error code(s)
> but forgot to add message(s). There's nothig to fix in sss_strerror.

yes, the underlying reason is that both lists are independent. What make
me think that sss_strerror() should be enhanced as well is that ERR_LAST
is a valid input and a call should make sure it does not segfault on
valid input.

bye,
Sumit

> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list