Bug Hunting Session
Bug 94695 - osl_createHostAddrByAddr uses non-threadsafe gethostbyaddr
Summary: osl_createHostAddrByAddr uses non-threadsafe gethostbyaddr
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Arkadiy Illarionov
URL:
Whiteboard: target:6.0.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2015-10-02 12:22 UTC by straub
Modified: 2017-10-29 21:26 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description straub 2015-10-02 12:22:19 UTC
gethostbyaddr is marked as obsolete and thread-unsafe in recent linux manpages. The corresponding code should be changed to use getaddrinfo or at least gethostbyaddr_r
Comment 1 Robinson Tryon (qubit) 2015-12-14 07:10:03 UTC Comment hidden (obsolete)
Comment 2 Björn Michaelsen 2016-01-26 17:26:41 UTC
removing skillLinux keyword. There is no such thing at https://wiki.documentfoundation.org/Development/Easy_Hacks/lists/by_Required_Skill and having a "skill*" keyword makes the easyHack not even appear on that page even in "uncategorized".
Comment 3 Robinson Tryon (qubit) 2016-02-18 14:52:16 UTC Comment hidden (obsolete)
Comment 4 Jakub Trzebiatowski 2016-03-01 11:07:02 UTC
What about windows? Is it thread-safe on windows?
Comment 5 jani 2016-07-06 06:01:37 UTC
sal/osl/unx/socket.cxx
Comment 6 Sudarshan K 2016-10-15 20:15:10 UTC
Submitted a patch here - https://gerrit.libreoffice.org/29909. I checked the links here - 
http://www.ibm.com/support/knowledgecenter/ssw_i5_54/rzab6/xthread.htm
and
https://linux.die.net/man/3/gethostbyaddr_r
gethostbyaddr_r is thread safe.
Comment 7 jani 2016-10-16 08:04:26 UTC
(In reply to Sudarshan K from comment #6)
> Submitted a patch here - https://gerrit.libreoffice.org/29909. I checked the
> links here - 
> http://www.ibm.com/support/knowledgecenter/ssw_i5_54/rzab6/xthread.htm
> and
> https://linux.die.net/man/3/gethostbyaddr_r
> gethostbyaddr_r is thread safe.

No need to comment in the bug, we see the patch in gerrit and it has already been commented and the tdf#<number> in the patch makes the connection.
Comment 8 jani 2016-10-16 08:05:26 UTC
Why have you set yourself as QA contact ? you have submitted a patch so surely you are assignee (and status == ASSIGNED)
Comment 9 Sudarshan K 2016-10-16 08:16:54 UTC
(In reply to jan iversen from comment #8)
> Why have you set yourself as QA contact ? you have submitted a patch so
> surely you are assignee (and status == ASSIGNED)

I saw that, I must have done it by mistake. I have taken myself off it. Although I have assigned myself the bug, since I'm working on it.
Comment 10 Sudarshan K 2016-10-16 08:48:23 UTC
Hey, so I'm stuck at a problem here. I will try to define it in a succinct manner.
1. gethostbyaddr and gethostbyaddr_r(its thread-safe but deprecated counterpart) both return a struct hostent. Consequently a lot of functions and datatypes internally have been defined using this struct hostent (e.g.hostentToHostAddr).

2. Whereas, getaddrinfo stores its results in a struct addrinfo. This in turn is used to iterate through the resultant addresses obtained. Suffice to say, addrinfo and hostent are very different.

The two possible ways to solve this is -
1. Try to convert the struct addrinfo into struct hostent. By that, I meant copy the resultant parameters in addrinfo with the ones in hostent. At this point, I don't even know if its possible or not. I will check forums/SO/man-pages etc to see. Or Maybe addrinfo can be made compatible with hostent (????)

2. Or modify the existing data-types/internal functions using hostent to use addrinfo instead. Or add new data-types/functions to use addrinfo. I doubt this would remain an easy-hack then. 

Or maybe I'm missing something and this hack isn't that complicated at all. In that case, kindly give me some hints to solve it. Thanks.
Comment 11 jani 2016-10-16 20:10:12 UTC
(In reply to Sudarshan K from comment #10)
> Hey, so I'm stuck at a problem here. I will try to define it in a succinct
> manner.
> 1. gethostbyaddr and gethostbyaddr_r(its thread-safe but deprecated
> counterpart) both return a struct hostent. Consequently a lot of functions
> and datatypes internally have been defined using this struct hostent
> (e.g.hostentToHostAddr).

I cannot follow that argument, if you decipher the structures follow the calls, you will see that only very limited info is used:

struct oslHostAddrImpl {
    555     rtl_uString     *pHostName;
    556     oslSocketAddr   pSockAddr;
    557 } ;

> 
> 2. Whereas, getaddrinfo stores its results in a struct addrinfo. This in
> turn is used to iterate through the resultant addresses obtained. Suffice to
> say, addrinfo and hostent are very different.
> 
> The two possible ways to solve this is -
> 1. Try to convert the struct addrinfo into struct hostent. By that, I meant
> copy the resultant parameters in addrinfo with the ones in hostent. At this
> point, I don't even know if its possible or not. I will check
> forums/SO/man-pages etc to see. Or Maybe addrinfo can be made compatible
> with hostent (????)
Yes you need to convert the information, but not be rebuilding the old structure, instead check what is actually needed.

> 
> 2. Or modify the existing data-types/internal functions using hostent to use
> addrinfo instead. Or add new data-types/functions to use addrinfo. I doubt
> this would remain an easy-hack then. 

Well even the base struct oslHostAddrImpl is not used a lot of places, but I would not change all these places, but instead generate oslHostAddrImpl from the information the new call delivers.

> 
> Or maybe I'm missing something and this hack isn't that complicated at all.
> In that case, kindly give me some hints to solve it. Thanks.
Comment 12 jani 2016-11-16 06:28:29 UTC
A polite ping still working on this bug ?
Comment 13 jani 2016-12-21 08:56:58 UTC
Unassigned due to lack of work
Comment 14 jani 2017-05-14 07:42:58 UTC Comment hidden (obsolete)
Comment 15 Xisco Faulí 2017-06-14 02:22:33 UTC Comment hidden (obsolete)
Comment 16 Xisco Faulí 2017-07-15 02:31:41 UTC Comment hidden (obsolete)
Comment 17 Xisco Faulí 2017-08-15 02:23:38 UTC Comment hidden (obsolete)
Comment 18 Xisco Faulí 2017-09-15 15:27:44 UTC Comment hidden (obsolete)
Comment 19 Xisco Faulí 2017-09-16 02:04:36 UTC Comment hidden (obsolete)
Comment 20 Xisco Faulí 2017-09-17 02:04:41 UTC Comment hidden (obsolete)
Comment 21 Arkadiy Illarionov 2017-09-26 18:52:18 UTC
Hi,
I work on this bug and use getnameinfo() instead of suggested getaddrinfo() since all we need from this is host's name. Am I missing something?
Comment 22 Arkadiy Illarionov 2017-10-27 06:05:24 UTC
Hi,
I'd like somebody review patch https://gerrit.libreoffice.org/#/c/42769/
Comment 23 Commit Notification 2017-10-29 14:52:12 UTC
Arkadiy Illarionov committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=a6750166cdd9b39f4e948201143c70ea95def600

tdf#94695 Replace gethostbyaddr with getnameinfo

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 24 Stephan Bergmann 2017-10-29 21:26:02 UTC
Thanks for the fix!