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