Bug 247102 - getlogin_r(): Sort of a typo: int is actually a size_t
Summary: getlogin_r(): Sort of a typo: int is actually a size_t
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: Unspecified
Hardware: Any Any
: --- Affects Some People
Assignee: Kyle Evans
URL: https://reviews.freebsd.org/D26335
Keywords: easy
Depends on:
Blocks:
 
Reported: 2020-06-09 06:10 UTC by Bertram Scharpf
Modified: 2020-09-15 04:26 UTC (History)
5 users (show)

See Also:
koobs: maintainer-feedback? (brooks)
koobs: maintainer-feedback? (wes)
kevans: mfc-stable12+
kevans: mfc-stable11-


Attachments
Just a small patch (754 bytes, patch)
2020-06-09 06:10 UTC, Bertram Scharpf
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bertram Scharpf 2020-06-09 06:10:08 UTC
Created attachment 215384 [details]
Just a small patch

The function getlogin_r() is declared with a second parameter of type int. This surely was meant to be a size_t.

See also:
contrib/llvm-project/llvm/include/llvm/Analysis/TargetLibraryInfo.def:889:/// int getlogin_r(char *name, size_t namesize)

Ruby 2.8 compilation was careried out on Linux, but breaks on FreeBSD because of this.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2020-06-09 06:18:27 UTC
^Triage: 

- brooks@ last touched around this area in base r332119 and may have some input on this, request feedback
- wes@ originally wrote it, do the same
Comment 2 Conrad Meyer freebsd_committer 2020-06-09 16:48:14 UTC
These functions are specified by POSIX as... taking size_t.  Yeah, our version seems wrong.  However, our libc implementation takes int, so to change this you need to provide libc ABI compatibility for the old version.
Comment 3 Conrad Meyer freebsd_committer 2020-06-09 16:56:15 UTC
Something like this:

--- a/lib/libc/gen/Symbol.map
+++ b/lib/libc/gen/Symbol.map
@@ -156,7 +156,6 @@ FBSD_1.0 {
        gethostname;
        getloadavg;
        getlogin;
-       getlogin_r;
        setnetgrent;
        getnetgrent;
        endnetgrent;
@@ -423,6 +422,7 @@ FBSD_1.5 {
 };

 FBSD_1.6 {
+       getlogin_r;
        memalign;
        sigandset;
        sigisemptyset;
--- a/lib/libc/gen/getlogin.c
+++ b/lib/libc/gen/getlogin.c
@@ -58,7 +58,7 @@ getlogin(void)
 }

 int
-getlogin_r(char *logname, int namelen)
+getlogin_r(char *logname, size_t namelen)
 {
        char tmpname[MAXLOGNAME];
        int     len;
@@ -75,3 +75,13 @@ getlogin_r(char *logname, int namelen)
        strlcpy(logname, tmpname, len);
        return (0);
 }
+
+/* FreeBSD 12 and earlier compat. */
+int
+__getlogin_r_fbsd12(char *logname, int namelen)
+{
+       if (namelen < 1)
+               return (ERANGE);
+       return (getlogin_r(logname, namelen));
+}
+__sym_compat(getlogin_r, __getlogin_r_fbsd12, FBSD_1.0);
Comment 4 Bertram Scharpf 2020-09-04 15:56:47 UTC
I posted this bug report almost three months ago. In the meantime, the fix works here through the whole time.

Will there ever happen anything further? Do you want to get rid of me? Sorry for trying to contribute.

If this post won't receive an answer, I will file another bug report about the bug report system.
Comment 5 Kyle Evans freebsd_committer 2020-09-04 16:09:45 UTC
(In reply to Bertram Scharpf from comment #4)

Hi,

Sorry, but there was no confirmation that Conrad's patch actually fixed the problem that spurred this PR. He fixed the problem he identified with the standard mismatch, but he received no feedback to confirm that there aren't any other unexpected issues (e.g. neither patch actually fixes the declaration in ^/include/unistd.h, for instance) -- based on this alone, I wouldn't have picked it up either.
Comment 6 Bertram Scharpf 2020-09-06 04:09:06 UTC
So, what has to happen that I can remove my local patch and still compile Ruby2.8?
Comment 7 Kyle Evans freebsd_committer 2020-09-06 04:12:36 UTC
(In reply to Bertram Scharpf from comment #6)

I've pushed the complete patch (your manage change + cem's ABI compat patch + header change) into phabricator at the above-referenced review URL. Once it's gotten another review or two, I'll commit it and MFC in a couple days to get it into 12.2.
Comment 8 Bertram Scharpf 2020-09-07 11:40:05 UTC
Thank you. This is the third time I was forgotten.
Comment 9 Ed Maste freebsd_committer 2020-09-08 02:46:32 UTC
To the original reporter, have you tried the combined patch as proposed (in D26335)?
Comment 10 commit-hook freebsd_committer 2020-09-09 18:07:22 UTC
A commit references this bug:

Author: kevans
Date: Wed Sep  9 18:07:14 UTC 2020
New revision: 365506
URL: https://svnweb.freebsd.org/changeset/base/365506

Log:
  getlogin_r: fix the type of len

  getlogin_r is specified by POSIX to to take a size_t len, not int. Fix our
  version to do the same, bump the symbol version due to ABI change and
  provide compat.

  This was reported to break compilation of Ruby 2.8.

  Some discussion about the necessity of the ABI compat did take place in the
  review. While many 64-bit platforms would likely be passing it in a 64-bit
  register and zero-extended and thus, not notice ABI breakage, some do
  sign-extend (e.g. mips).

  PR:		247102
  Submitted by:	Bertram Scharpf <software@bertram-scharpf.de> (original)
  Submitted by:	cem (ABI compat)
  MFC after:	1 week
  Differential Revision:	https://reviews.freebsd.org/D26335

Changes:
  head/include/unistd.h
  head/lib/libc/gen/Symbol.map
  head/lib/libc/gen/getlogin.c
  head/lib/libc/sys/getlogin.2
Comment 11 commit-hook freebsd_committer 2020-09-13 01:45:08 UTC
A commit references this bug:

Author: kevans
Date: Sun Sep 13 01:44:32 UTC 2020
New revision: 365682
URL: https://svnweb.freebsd.org/changeset/base/365682

Log:
  MFC r365506 getlogin_r: fix the type of len

  getlogin_r is specified by POSIX to to take a size_t len, not int. Fix our
  version to do the same, bump the symbol version due to ABI change and
  provide compat.

  This was reported to break compilation of Ruby 2.8.

  Some discussion about the necessity of the ABI compat did take place in the
  review. While many 64-bit platforms would likely be passing it in a 64-bit
  register and zero-extended and thus, not notice ABI breakage, some do
  sign-extend (e.g. mips).

  PR:		247102

Changes:
_U  stable/12/
  stable/12/include/unistd.h
  stable/12/lib/libc/gen/Symbol.map
  stable/12/lib/libc/gen/getlogin.c
  stable/12/lib/libc/sys/getlogin.2
Comment 12 commit-hook freebsd_committer 2020-09-13 02:18:19 UTC
A commit references this bug:

Author: kevans
Date: Sun Sep 13 02:17:59 UTC 2020
New revision: 365684
URL: https://svnweb.freebsd.org/changeset/base/365684

Log:
  MFS r365682: getlogin_r: fix the type of len

  getlogin_r is specified by POSIX to to take a size_t len, not int. Fix our
  version to do the same, bump the symbol version due to ABI change and
  provide compat.

  This was reported to break compilation of Ruby 2.8.

  Some discussion about the necessity of the ABI compat did take place in the
  review. While many 64-bit platforms would likely be passing it in a 64-bit
  register and zero-extended and thus, not notice ABI breakage, some do
  sign-extend (e.g. mips).

  PR:		247102
  Approved by:	re (gjb)

Changes:
_U  releng/12.2/
  releng/12.2/include/unistd.h
  releng/12.2/lib/libc/gen/Symbol.map
  releng/12.2/lib/libc/gen/getlogin.c
  releng/12.2/lib/libc/sys/getlogin.2
Comment 13 Kyle Evans freebsd_committer 2020-09-13 02:20:13 UTC
Merged all the way to releng/12.2; thanks!