Closed
Bug 322956
Opened 19 years ago
Closed 19 years ago
The PR_GetSystemInfo/ needs hook to return untruncated hostname
Categories
(NSPR :: NSPR, enhancement)
NSPR
NSPR
Tracking
(Not tracked)
RESOLVED
FIXED
4.6.3
People
(Reporter: philipp, Assigned: wtc)
References
Details
(Keywords: fixed1.8.1)
Attachments
(1 file, 3 obsolete files)
2.25 KB,
patch
|
philipp
:
review+
darin.moz
:
superreview+
dbaron
:
approval1.8.1+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.12) Gecko/20050922 Fedora/1.0.7-1.1.fc3 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.12) Gecko/20050922 Fedora/1.0.7-1.1.fc3 Firefox/1.0.7
The API to this functionality is overly restrictive: it currently only returns the leftmost portion of the hostname (up to the first dot, i.e. stripping any domain name).
There is no way to get the unmodified hostname as configured on the system.
This bug presents a fix for this.
Reproducible: Always
Steps to Reproduce:
1. at the root prompt, set the hostname to a FQDN
2. Call PR_GetSystemInfo(PR_SI_HOSTNAME, ...)
3. print the return value
Actual Results:
Only the host portion of the name is returned.
Expected Results:
It should return the FQDN unmodified.
Reporter | ||
Comment 1•19 years ago
|
||
Fix to prsystem.c, plus to calling code.
Attachment #208108 -
Flags: review?(bienvenu)
Reporter | ||
Comment 2•19 years ago
|
||
Hmm.... Is it worth removing the definitions of _PR_GET_HOST_ADDR_AS_NAME if it's no longer used?
Comment 3•19 years ago
|
||
Comment on attachment 208108 [details] [diff] [review]
Proposed fix
this is all code I'm not an owner of. darin@meer.net owns the netwerk code, wtc@redhat.com (?) owns nspr, and I think kaie owns the security code. I have no idea who owns the xremote code...
Attachment #208108 -
Flags: review?(bienvenu)
Updated•19 years ago
|
Assignee: mscott → wtchang
Component: General → NSPR
Product: Thunderbird → NSPR
QA Contact: general → wtchang
Version: unspecified → other
Reporter | ||
Updated•19 years ago
|
Attachment #208108 -
Flags: review?(darin)
Assignee | ||
Comment 4•19 years ago
|
||
I can't change what PR_GetSystemInfo/PR_SI_HOSTNAME returns.
We'd need to add a new PR_SI_FQDN flag. I'm not sure if we
can get the FQDN on all platforms.
Reporter | ||
Comment 5•19 years ago
|
||
(In reply to comment #4)
> I can't change what PR_GetSystemInfo/PR_SI_HOSTNAME returns.
> We'd need to add a new PR_SI_FQDN flag. I'm not sure if we
> can get the FQDN on all platforms.
>
Why can't it be changed?
And as far as I know, "gethostname()" should return whatever the user set, whether that's FQDN or just the host portion. Isn't that a local configuration issue?
I can change the patch if you want to add an additional type, but... it seems like it would be duplicating code for nothing.
We could also add a wrapper that zaps the dotted portion off, and change the existing code to go through the wrapper.
Comment 6•19 years ago
|
||
It can't be changed because backwards compatibility must be ensured: there are other users of NSPR and breaking them is not an option.
Reporter | ||
Comment 7•19 years ago
|
||
(In reply to comment #6)
> It can't be changed because backwards compatibility must be ensured: there are
> other users of NSPR and breaking them is not an option.
What other users? I went through the sources and changed them all to maintain continuity of the behavior.
Are these users outside the source tree?
Comment 8•19 years ago
|
||
Yes.
Assignee | ||
Comment 9•19 years ago
|
||
Philip: NSPR (and the NSS crypto libraries that I also
work on) is a library that has users outside the Mozilla
project. We provide backward compatibility at the binary
interface level. As a result there are limitations to
the kind of changes we can make. Very often the "desired"
behavior needs to be implemented as new functions or
flags rather than fixing the existing functions or flags.
Reporter | ||
Comment 10•19 years ago
|
||
(In reply to comment #9)
Sigh. Ok. Understood.
I'll add a new function.
Comment 11•19 years ago
|
||
Just to add my opinion (fir Unix/Linux).
Hostname is just the hostname. The hostname on Unix/Linux has no domain part.
On Unix/Linux you can have different domain-parts when it comes to for which service you speak:
There are:
hostname
domainname (which correlates to the NIS/YP domainname
dnsdomainname (this invokes the resolver library of the libc
All what I want to say is that PR_SI_HOSTNAME does exactly what it's supposed to do.
Reporter | ||
Comment 12•19 years ago
|
||
(In reply to comment #11)
> Just to add my opinion (fir Unix/Linux).
>
> Hostname is just the hostname. The hostname on Unix/Linux has no domain part.
You sure about that? At my site, it's the FQDN for each machine. I'm running Linux FC3.
As I remember, Solaris 2.8 also includes the domain portion into "hostname".
> On Unix/Linux you can have different domain-parts when it comes to for which
> service you speak:
> There are:
> hostname
> domainname (which correlates to the NIS/YP domainname
> dnsdomainname (this invokes the resolver library of the libc
You left out uname -n... which, by the way, also includes the domain name on all of my systems.
> All what I want to say is that PR_SI_HOSTNAME does exactly what it's supposed
> to do.
Are you sure? Because gethostname(), on my system, returns an FQDN name. And PR_SI_HOSTNAME is only supposed to be a wrapper for that function.
Comment 13•19 years ago
|
||
(In reply to comment #12)
> > Hostname is just the hostname. The hostname on Unix/Linux has no domain part.
>
> You sure about that? At my site, it's the FQDN for each machine. I'm running
> Linux FC3.
OK, that's a problem that there seems to be no standard about it.
RedHat/Fedora always set the FQDN for hostname while others just the the hostname for hostname. For example SUSE and Debian set only the hostname.
The manpage for hostname(1) says:
"hostname will print the name of the system as returned by the gethostname(2) function.
dnsdomainname will print the domain part of the FQDN (Fully Qualified Domain Name). The complete FQDN of the system is returned with hostname --fqdn."
Honestly I think that RH/Fedora is wrong with their setting but anyway it makes no difference for this bug since we would need a new flag/function anyway for the FQDN.
Reporter | ||
Comment 14•19 years ago
|
||
(In reply to comment #13)
> Honestly I think that RH/Fedora is wrong with their setting but anyway it makes
> no difference for this bug since we would need a new flag/function anyway for
> the FQDN.
Regrettably, that's all I have to test with.
Have a look at the latest patches. Maybe someone can come up with a patch for non-RH, non-Solaris machines.
Reporter | ||
Comment 15•19 years ago
|
||
(In reply to comment #13)
I just looked on a FreeBSD system, and that too uses hostname to store the FQDN.
I'm starting to suspect that the couple of systems that you use are the exception and not the rule.
Comment 16•19 years ago
|
||
(In reply to comment #15)
> I just looked on a FreeBSD system, and that too uses hostname to store the
> FQDN.
>
> I'm starting to suspect that the couple of systems that you use are the
> exception and not the rule.
There is no need to discuss this here anymore. NSPR has a defined behaviour which cannot be changed that way for existing interfaces. And we are not a standardization instance.
Reporter | ||
Comment 17•19 years ago
|
||
(In reply to comment #16)
> There is no need to discuss this here anymore. NSPR has a defined behaviour
> which cannot be changed that way for existing interfaces. And we are not a
> standardization instance.
See comment #4, and the latest patch diffs.
We've added PR_SI_HOSTNAME_FQDN instead of changing the existing semantics of PR_SI_HOSTNAME.
A "standardization instance"? How's that? Do you mean a standards organization?
Comment 18•19 years ago
|
||
(In reply to comment #17)
> See comment #4, and the latest patch diffs.
I never objected to that. I have commented on your first request which is still the summary of this bug. But I can't see your latest patch which adds the following. Where is it?
> We've added PR_SI_HOSTNAME_FQDN instead of changing the existing semantics of
> PR_SI_HOSTNAME.
yeah, it's OK for me ;-)
> A "standardization instance"? How's that? Do you mean a standards
> organization?
yes
Reporter | ||
Comment 19•19 years ago
|
||
Attachment #208108 -
Attachment is obsolete: true
Attachment #208184 -
Flags: review?(wtchang)
Attachment #208108 -
Flags: review?(darin)
Reporter | ||
Comment 20•19 years ago
|
||
Sorry. Spaced and left the diffs out. Now attached...
I guess I got too busy updating 279525 instead, to incorporate this modified approach.
I still don't get the reference to standards bodies: how many difference ways are there to retrieve the FQDN? We could use uname(), I suppose... But I don't know how that works on MacOS or XP.
Comment 21•19 years ago
|
||
(In reply to comment #20)
> I still don't get the reference to standards bodies: how many difference ways
> are there to retrieve the FQDN? We could use uname(), I suppose... But I
> don't know how that works on MacOS or XP.
you can't use uname because it just knows what's set as hostname. And what's set as hostname is not defined to be the FQDN. I think on Linux you really have to ask the resolver to get it and then it could still be broken if the resolver isn't configured correctly.
You would have to go for gethostbyname(hostname)->h_name
Reporter | ||
Comment 22•19 years ago
|
||
(In reply to comment #21)
> you can't use uname because it just knows what's set as hostname. And what's
> set as hostname is not defined to be the FQDN. I think on Linux you really have
> to ask the resolver to get it and then it could still be broken if the resolver
> isn't configured correctly.
> You would have to go for gethostbyname(hostname)->h_name
I'm not sure that's entirely true.
In cases of using NIS, NIS+, ActiveDirectory, dyndns, or /etc/hosts files means that you can't rely on traditional DNS being the only behavior you will encounter. Especially in private networks and/or NATting.
Assignee | ||
Comment 23•19 years ago
|
||
Philip: what problem are you trying to solve? You didn't
say what (new) code needs the unmodified hostname as
configured on the system.
Comment 24•19 years ago
|
||
(In reply to comment #23)
> Philip: what problem are you trying to solve? You didn't
> say what (new) code needs the unmodified hostname as
> configured on the system.
I believe Philip intended to use it for the blocked bug: bug 279525.
Assignee | ||
Comment 25•19 years ago
|
||
Comment on attachment 208184 [details] [diff] [review]
Fix, using an additional enum to access the untruncated value.
This new flag should be named PR_SI_HOSTNAME_UNTRUNCATED
rather than PR_SI_HOSTNAME_FQDN because your patch doesn't
try to get the FQDN. (I suspect your application doesn't
need the FQDN.) We can also add PR_SI_HOSTNAME_TRUNCATED
as a synonym for PR_SI_HOSTNAME.
To unblock bug 279525, you can just call the system library
function gethostname(). I verified that PR_SI_HOSTNAME
calls gethostname() on all platforms. The only issue you
need to deal with is to include the right system header
file (<unistd.h> for Unix/Linux/Mac OS X, <windows.h> or
<winsock2.h> for Windows). Very likely, the files you
need to modify already include the system header file
that declares gethostname().
Attachment #208184 -
Flags: review?(wtchang) → review+
Comment 26•19 years ago
|
||
(In reply to comment #22)
> > You would have to go for gethostbyname(hostname)->h_name
>
> I'm not sure that's entirely true.
>
> In cases of using NIS, NIS+, ActiveDirectory, dyndns, or /etc/hosts files means
> that you can't rely on traditional DNS being the only behavior you will
> encounter. Especially in private networks and/or NATting.
You should read first what gethostbyname actually does!
The system's resolver always works on a properly configured system because it isn't bound to DNS. On older systems it refers to /etc/host.conf (libc5) and on newer ones (glibc) to /etc/nsswitch.conf.
I still understand that you search for the FQDN. So you should really use gethostbyname().
Reporter | ||
Comment 27•19 years ago
|
||
(In reply to comment #26)
> I still understand that you search for the FQDN. So you should really use
> gethostbyname().
No, I'm not looking for the name that the machine is known by on the network, because it doesn't matter. Historically, HELO was only needed to stop machines from talking back to themselves and creating single-node mail routing loops, after all...
I'm looking for the name that the machine knows itself to be. This is an important distinction.
Imagine that I take my laptop to work with me, sit down at my desk, and plug it into the network. I should be able to send and receive email immediately, well before DHCP, Active Directories, Dynamic DNS, or LDAP has had a chance to propagate state throughout the network (which could take several minutes... or even hours, if a network partition happens between a primary and secondary server).
You're espousing that I trust the network (whether it's DNS, NIS+, Active Directories, LDAP, or whatever).
I'm saying I prefer local state, because I can control it and verify it... and it's less susceptible to malicious manipulation.
You're also saying that identity is a function of address (hence the suggestion to use address-to-name mapping via the gethostbyname(), presumably passing in a dotted-quad address) function.
This doesn't work for two reasons:
(a) It ignores the fact that identity (read: "name") might be constant, and independent of address. I.e. what if the network gives me an address, but I tell the network, "Ok, address x.x.x.x is now bound to myname.dom.ain", where myname.dom.ain is a constant name assigned to my laptop.
(b) A lot of VPN software will (1) maintain two different interfaces, one being a physical interface, and the other being a tunnel end-point, and (2) even a fairly sophisticated application might not be able to determine which of the two interfaces corresponds to "home".
A common scenario when I worked at Cisco was to use VPN split-tunnelling when working from home. If I was contacting an internal mail server for work-related email, then my connection would be bound to vpn0, gre0, ipsec0, or whatever the implementation happened to call my tunnel interface.
If I was retrieving personal email on the same machine, connections to destinations not on the corporate subnet would flow out the default route interface, which might be eth0, ppp0, etc. pointing at my ISP.
In other words, using two different accounts in the same TB session, I could be talking to two different mail servers with two different local IP addresses simultaneously.
How do I know which address to use?
Trick question.
The answer is (a) you can't, and (b) you don't really care.
You go by your statically configured host name.
And lastly, with the advent of wireless networks (GPRS, Wimax, etc) you have roaming, where a machine might change networks and I addresses several times during the course of a single session. Should its identity be constantly revolving to match the address-to-name binding?
No, of course not.
Don't fall into the trap of looking at the world as a bunch of dial-up users, with changing addresses, and fixed address-to-name mappings.
That's actually a only a small percentage of the total number of scenarios that we have to engineer working solutions for.
The world is so much more complicated than that.
Reporter | ||
Comment 28•19 years ago
|
||
(In reply to comment #25)
> (From update of attachment 208184 [details] [diff] [review] [edit])
> This new flag should be named PR_SI_HOSTNAME_UNTRUNCATED
> rather than PR_SI_HOSTNAME_FQDN because your patch doesn't
> try to get the FQDN. (I suspect your application doesn't
> need the FQDN.) We can also add PR_SI_HOSTNAME_TRUNCATED
> as a synonym for PR_SI_HOSTNAME.
I will change the name.
> To unblock bug 279525, you can just call the system library
> function gethostname(). I verified that PR_SI_HOSTNAME
> calls gethostname() on all platforms. The only issue you
> need to deal with is to include the right system header
> file (<unistd.h> for Unix/Linux/Mac OS X, <windows.h> or
> <winsock2.h> for Windows). Very likely, the files you
> need to modify already include the system header file
> that declares gethostname().
Ironically, I originally submitted the patch to 279525 to use gethostname() directly, but was told that a more portable fix would be desirable, and hence to use the PR_GetSystemInfo() interface instead.
It might be conceivable that some future platform won't support gethostname(), but only uname()... so I'd like to stick with a common interface via PR_GetSystemInfo().
Besides, conditional compilation is fine if you limit it to "glue" or "shim" files, but when the main body of code starts getting cluttered with it, things get unreadable very quickly.
Reporter | ||
Updated•19 years ago
|
Summary: The PR_GetSystemInfo/PR_SI_HOSTNAME interface truncates the hostname → The PR_GetSystemInfo/ needs hook to return untruncated hostname
Comment 29•19 years ago
|
||
OK, so you really want what is defined as hostname. This could be a configured FQDN-like string with dots or just the hostname without domain part.
This is what you have to know with the current approach. The only thing I want to make clear is that you might not get the complete hostname as configured in /etc/HOSTNAME but only the first part.
Reporter | ||
Comment 30•19 years ago
|
||
(In reply to comment #29)
> OK, so you really want what is defined as hostname. This could be a configured
> FQDN-like string with dots or just the hostname without domain part.
> This is what you have to know with the current approach. The only thing I want
> to make clear is that you might not get the complete hostname as configured in
> /etc/HOSTNAME but only the first part.
Correct.
I don't know if there is a truly portable way to always get what the machine thinks is its static name. FreeBSD, Solaris, Mach, and Fedora all store it in the hostname.
Reporter | ||
Comment 31•19 years ago
|
||
Changed the enum to PR_SI_HOSTNAME_UNTRUNCATED ...
Attachment #208433 -
Flags: review?(wtchang)
Reporter | ||
Updated•19 years ago
|
Attachment #208184 -
Attachment is obsolete: true
Assignee | ||
Comment 32•19 years ago
|
||
Comment on attachment 208433 [details] [diff] [review]
Integrated the suggestion to change the enum name...
r=wtc. The only thing missing is documentation that
explains the difference between PR_SI_HOSTNAME and
PR_SI_HOSTNAME_TRUNCATED. I will propose some wording
in the subsequent patch.
Attachment #208433 -
Flags: review?(wtchang) → review+
Assignee | ||
Comment 33•19 years ago
|
||
Philip, please review the comments I added to explain
PR_SI_HOSTNAME and the new PR_SI_HOSTNAME_UNTRUNCATED
flag.
Darin, this patch adds a new PR_SI_HOSTNAME_UNTRUNCATED
flag for PR_GetSystemInfo to get the hostname as configured
on the system (i.e., what hostname() returns, unmodified).
Attachment #208433 -
Attachment is obsolete: true
Attachment #208678 -
Flags: superreview?(darin)
Attachment #208678 -
Flags: review?(philipp)
Reporter | ||
Comment 34•19 years ago
|
||
Looks good.
Let's do it!
Thanks.
Reporter | ||
Comment 35•19 years ago
|
||
Do I need to sign off on this? And if so, how?
Comment 36•19 years ago
|
||
(In reply to comment #35)
> Do I need to sign off on this? And if so, how?
Just set the "review" dropdown to "+", at https://bugzilla.mozilla.org/attachment.cgi?id=208678&action=edit .
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 37•19 years ago
|
||
(In reply to comment #36)
> (In reply to comment #35)
> > Do I need to sign off on this? And if so, how?
>
> Just set the "review" dropdown to "+", at
> https://bugzilla.mozilla.org/attachment.cgi?id=208678&action=edit .
I tried that, but it tells me that I'm not authorized to modify the attachment.
Comment 38•19 years ago
|
||
(In reply to comment #37)
> I tried that, but it tells me that I'm not authorized to modify the attachment.
Sorry, I forgot that you need special privileges for that. Your Bugzilla privileges have now been set correctly, you should be able to edit it now.
Reporter | ||
Updated•19 years ago
|
Attachment #208678 -
Flags: review?(philipp) → review+
Comment 39•19 years ago
|
||
Comment on attachment 208678 [details] [diff] [review]
Proposed patch v3
looks good to me.
Attachment #208678 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 40•19 years ago
|
||
I checked in the patch on the NSPR trunk (NSPR 4.7) and
the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.9 alpha).
Checking in include/prsystem.h;
/cvsroot/mozilla/nsprpub/pr/include/prsystem.h,v <-- prsystem.h
new revision: 3.11; previous revision: 3.10
done
Checking in src/misc/prsystem.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prsystem.c,v <-- prsystem.c
new revision: 3.28; previous revision: 3.27
done
Checking in include/prsystem.h;
/cvsroot/mozilla/nsprpub/pr/include/prsystem.h,v <-- prsystem.h
new revision: 3.7.4.4; previous revision: 3.7.4.3
done
Checking in src/misc/prsystem.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prsystem.c,v <-- prsystem.c
new revision: 3.16.4.10; previous revision: 3.16.4.9
done
Severity: normal → enhancement
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
Comment 41•19 years ago
|
||
Comment on attachment 208678 [details] [diff] [review]
Proposed patch v3
if we're going to want the ehlo change for tb 2.0, we'd need this change on the 1.8.1 branch - it seems safe, since it's just adding an enum and some corresponding code...
Attachment #208678 -
Flags: approval1.8.1?
Comment on attachment 208678 [details] [diff] [review]
Proposed patch v3
Assuming that wtc is ok with the landing on the branch, a=dbaron on behalf of drivers. Please check in to MOZILLA_1_8_BRANCH and marked fixed1.8.1 when you have done so.
Attachment #208678 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Comment 43•19 years ago
|
||
I checked in the proposed patch v3 on the NSPR_4_6_BRANCH
(NSPR 4.6.3 Beta) and MOZILLA_1_8_BRANCH (Mozilla 1.8.1 Beta 2).
Checking in include/prsystem.h;
/cvsroot/mozilla/nsprpub/pr/include/prsystem.h,v <-- prsystem.h
new revision: 3.10.2.1; previous revision: 3.10
done
Checking in src/misc/prsystem.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prsystem.c,v <-- prsystem.c
new revision: 3.26.2.2; previous revision: 3.26.2.1
done
Checking in include/prsystem.h;
/cvsroot/mozilla/nsprpub/pr/include/prsystem.h,v <-- prsystem.h
new revision: 3.7.4.3.6.1; previous revision: 3.7.4.3
done
Checking in src/misc/prsystem.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prsystem.c,v <-- prsystem.c
new revision: 3.16.4.7.6.3; previous revision: 3.16.4.7.6.2
done
Keywords: fixed1.8.1
Target Milestone: 4.7 → 4.6.3
You need to log in
before you can comment on or make changes to this bug.
Description
•