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 <[email protected]> > >> >>>>>>>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 > [email protected] > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
