[Freeipa-devel] [PATCHES] 206-209 Add default CFLAGS & fix hardened build

[Jan Cholasta]

On 6.12.2013 13:31, Jan Cholasta wrote:
> On 6.12.2013 12:57, Alexander Bokovoy wrote:
>> On Fri, 06 Dec 2013, Petr Viktorin wrote:
>>> On 12/06/2013 11:52 AM, Jan Cholasta wrote:
>>>> On 5.12.2013 13:31, Alexander Bokovoy wrote:
>>>>> On Thu, 05 Dec 2013, Petr Viktorin wrote:
>>>>>> On 12/05/2013 11:15 AM, Jan Cholasta wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> the attached patches fix
>>>>>>> <https://fedorahosted.org/freeipa/ticket/3896>.
>>>>>>>
>>>>>>> Patch 207 should fix build failures some of you were having after
>>>>>>> hardenening was enabled in the spec file.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> In 209, would (ret != 1) make more sense than (ret == -1)? AFAIU zero
>>>>>> would also indicate a problem, if it somehow ended up being returned.
>>>>> no, write() returns -1 if an error has happened and amount of the data
>>>>> written otherwise. We specifically need to check that there was an
>>>>> error, not that we wrote more or less than were supposed to write.
>>>>>
>>>>> We are looking for EPIPE and EINTR mostly, since other errors make
>>>>> little
>>>>> difference for our case. In case of EINTR we need to repeat or the
>>>>> worker thread didn't receive our shutdown request. In case of EPIPE we
>>>>> will also get SIGPIPE and in general this means the other thread
>>>>> died..
>>>>>
>>>>> However, even if writing to the pipe failed, we still need to wait
>>>>> until
>>>>> thread dies with pthread_join(). I think returning -1 here is
>>>>> premature.
>>>>
>>>> Fixed, updated patches attached.
>>>>
>>>> Also removed CFLAGS duplication, see patch 212.
>>>
>>> Thanks you! The build works fine, ACK for 206-208, 212.
>>>
>>> Alexander, is the C part OK? It seems a do-while loop would make sense
>>> here.
>> I think do-while would be cleaner, purely from intent expression point
>> of view.
>>
>
> I actually like it the way it is, because it follows the
>
>     ret = func()
>     if (ret == -1) {
>     }
>
> pattern, which is used abundantly in our code.
>

... but I don't have a strong opinion about this, so whatever floats 
your boat.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-206.2-Prefer-user-CFLAGS-CPPFLAGS-over-those-provided-by-r.patch
Type: text/x-patch
Size: 854 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/ef1780ef/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-207.2-Include-LDFLAGS-provided-by-rpmbuild-in-global-LDFLA.patch
Type: text/x-patch
Size: 1367 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/ef1780ef/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-208.2-Add-stricter-default-CFLAGS-to-Makefile.patch
Type: text/x-patch
Size: 781 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/ef1780ef/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-209.2-Fix-compilation-error-in-ipa-cldap.patch
Type: text/x-patch
Size: 960 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/ef1780ef/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-212.2-Remove-CFLAGS-duplication.patch
Type: text/x-patch
Size: 8924 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131206/ef1780ef/attachment-0004.bin>