[PATCH] Fix /proc pathname sizes on Solaris

[PATCH] Fix /proc pathname sizes on Solaris

[Rainer Orth]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

I'm slowly working my way through the gdb patches from the
solaris-userland repo

	https://github.com/oracle/solaris-userland/tree/master/components/gdb/patches

Some of them are pretty obvious and should be able to go in (such as
this one and the next), while others are either incomplete
(e.g. 008-syscalls.patch, which adds XML descriptions of the Solaris
syscalls, but lacks their registration) or inappropriate in their
current form (unnecessarily intrusive).

This one (001-fix-proc-name-size.patch) should be obvious given the
patches' comment:

# In Solaris, PID_MAX is 999999 (6 digit pid).
# In Solaris, lwpid_t is an unsigned int, so theoretically the lwp id
# could be 10 digits.

Two questions about procedure here:

* AFAIK Oracle has a corporate copyright assignment on file, so the
  patches should be covered.  Even if that were not the case, this one
  and the next are certainly below the 15-line limit for non-trivial
  patches.

* Given the code isn't mine, how should we handle attribution?  I
  suspect the engineer who committed the patch to github is the author,
  but don't know for certain.  Should I attribute it to her in the
  ChangeLog?

Tested on i386-pc-solaris2.11.  Ok for master?

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2018-06-27  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* procfs.c (MAX_PROC_NAME_SIZE): Allow for 6-digit PID_MAX and
	uint_t lwpid_t.
	(create_procinfo): Print pids in /proc without leading zeros.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sol2-proc-pathname-size.patch --]
[-- Type: text/x-patch, Size: 855 bytes --]

# HG changeset patch
# Parent  e6140f0a7128422be8a7e2a148da8de516d676d8
Fix /proc pathname sizes on Solaris

diff --git a/gdb/procfs.c b/gdb/procfs.c
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -233,7 +233,7 @@ enum { READ_WATCHFLAG  = WA_READ,
 #define AS_PROC_NAME_FMT     "/proc/%d/as"
 #define MAP_PROC_NAME_FMT    "/proc/%d/map"
 #define STATUS_PROC_NAME_FMT "/proc/%d/status"
-#define MAX_PROC_NAME_SIZE sizeof("/proc/99999/lwp/8096/lstatus")
+#define MAX_PROC_NAME_SIZE sizeof("/proc/999999/lwp/0123456789/lwpstatus")
 
 typedef struct procinfo {
   struct procinfo *next;
@@ -483,7 +483,7 @@ create_procinfo (int pid, int tid)
     }
   else
     {
-      sprintf (pi->pathname, "/proc/%05d/lwp/%d", pid, tid);
+      sprintf (pi->pathname, "/proc/%d/lwp/%d", pid, tid);
       pi->next = parent->thread_list;
       parent->thread_list = pi;
     }

Re: [PATCH] Fix /proc pathname sizes on Solaris

[Joel Brobecker]

Hi Rainer,

On Mon, Sep 17, 2018 at 04:11:35PM +0200, Rainer Orth wrote:
> I'm slowly working my way through the gdb patches from the
> solaris-userland repo
> 
> 	https://github.com/oracle/solaris-userland/tree/master/components/gdb/patches
> 
> Some of them are pretty obvious and should be able to go in (such as
> this one and the next), while others are either incomplete
> (e.g. 008-syscalls.patch, which adds XML descriptions of the Solaris
> syscalls, but lacks their registration) or inappropriate in their
> current form (unnecessarily intrusive).
> 
> This one (001-fix-proc-name-size.patch) should be obvious given the
> patches' comment:
> 
> # In Solaris, PID_MAX is 999999 (6 digit pid).
> # In Solaris, lwpid_t is an unsigned int, so theoretically the lwp id
> # could be 10 digits.
> 
> Two questions about procedure here:
> 
> * AFAIK Oracle has a corporate copyright assignment on file, so the
>   patches should be covered.  Even if that were not the case, this one
>   and the next are certainly below the 15-line limit for non-trivial
>   patches.

I checked, and indeed, Oracle has a copyright assignment.

> * Given the code isn't mine, how should we handle attribution?  I
>   suspect the engineer who committed the patch to github is the author,
>   but don't know for certain.  Should I attribute it to her in the
>   ChangeLog?

Can you ask the user in question if they are the author? If not,
can they help figuring out who it is? Ideally, we would want the
name and email of the author of  the patch -- not sure what we should
be doing if we don't have that info.

> Tested on i386-pc-solaris2.11.  Ok for master?
> 
> 	Rainer
> 
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
> 
> 
> 2018-06-27  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> 
> 	* procfs.c (MAX_PROC_NAME_SIZE): Allow for 6-digit PID_MAX and
> 	uint_t lwpid_t.
> 	(create_procinfo): Print pids in /proc without leading zeros.

OK for me.

> # HG changeset patch
> # Parent  e6140f0a7128422be8a7e2a148da8de516d676d8
> Fix /proc pathname sizes on Solaris
> 
> diff --git a/gdb/procfs.c b/gdb/procfs.c
> --- a/gdb/procfs.c
> +++ b/gdb/procfs.c
> @@ -233,7 +233,7 @@ enum { READ_WATCHFLAG  = WA_READ,
>  #define AS_PROC_NAME_FMT     "/proc/%d/as"
>  #define MAP_PROC_NAME_FMT    "/proc/%d/map"
>  #define STATUS_PROC_NAME_FMT "/proc/%d/status"
> -#define MAX_PROC_NAME_SIZE sizeof("/proc/99999/lwp/8096/lstatus")
> +#define MAX_PROC_NAME_SIZE sizeof("/proc/999999/lwp/0123456789/lwpstatus")
>  
>  typedef struct procinfo {
>    struct procinfo *next;
> @@ -483,7 +483,7 @@ create_procinfo (int pid, int tid)
>      }
>    else
>      {
> -      sprintf (pi->pathname, "/proc/%05d/lwp/%d", pid, tid);
> +      sprintf (pi->pathname, "/proc/%d/lwp/%d", pid, tid);

I am wondering how this ever worked for processes whose pid had
fewer than 5 digits. I was initially concerned that this patch
introduced a change of behavior that would create an incompatibility.
But looking at Solaris 2.8 and 2.11 systems, I see processes with
3 or 4 digits PIDs, and the path in /proc doesn't have leading zeroes.

I also checked whether the file might be used on platforms other than
Solaris (see configure.nat), and this does not appear to be the case.

>        pi->next = parent->thread_list;
>        parent->thread_list = pi;
>      }


-- 
Joel

Re: [PATCH] Fix /proc pathname sizes on Solaris

[Rainer Orth]

Hi Joel,

>> * Given the code isn't mine, how should we handle attribution?  I
>>   suspect the engineer who committed the patch to github is the author,
>>   but don't know for certain.  Should I attribute it to her in the
>>   ChangeLog?
>
> Can you ask the user in question if they are the author? If not,
> can they help figuring out who it is? Ideally, we would want the
> name and email of the author of  the patch -- not sure what we should
> be doing if we don't have that info.

I'll try, but she's left Oracle since (mail just bounced).  Maybe I can
figure it out nonetheless through different contacts.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Fix /proc pathname sizes on Solaris

[Rainer Orth]

Hi Joel,

> On Mon, Sep 17, 2018 at 04:11:35PM +0200, Rainer Orth wrote:
>> I'm slowly working my way through the gdb patches from the
>> solaris-userland repo
>> 
>> 	https://github.com/oracle/solaris-userland/tree/master/components/gdb/patches
>> 
>> Some of them are pretty obvious and should be able to go in (such as
>> this one and the next), while others are either incomplete
>> (e.g. 008-syscalls.patch, which adds XML descriptions of the Solaris
>> syscalls, but lacks their registration) or inappropriate in their
>> current form (unnecessarily intrusive).
>> 
>> This one (001-fix-proc-name-size.patch) should be obvious given the
>> patches' comment:
>> 
>> # In Solaris, PID_MAX is 999999 (6 digit pid).
>> # In Solaris, lwpid_t is an unsigned int, so theoretically the lwp id
>> # could be 10 digits.
>> 
>> Two questions about procedure here:
>> 
>> * AFAIK Oracle has a corporate copyright assignment on file, so the
>>   patches should be covered.  Even if that were not the case, this one
>>   and the next are certainly below the 15-line limit for non-trivial
>>   patches.
>
> I checked, and indeed, Oracle has a copyright assignment.
>
>> * Given the code isn't mine, how should we handle attribution?  I
>>   suspect the engineer who committed the patch to github is the author,
>>   but don't know for certain.  Should I attribute it to her in the
>>   ChangeLog?
>
> Can you ask the user in question if they are the author? If not,
> can they help figuring out who it is? Ideally, we would want the
> name and email of the author of  the patch -- not sure what we should
> be doing if we don't have that info.

I've done some more digging myself: here's what I found:

* The MAX_PROC_NAME_SIZE part (done slightly differently) was originally
  done by Stefan Teleman when he imported gdb 7.6 into the userland repo
  (gdb.procfs.c.patch).

* The change to create_procinfo originated with April Chin when
  importing gdb 7.12.1 later (001-fix-proc-name-size.patch).

* I had to make minor adjustments for master to account for my removal
  of !NEW_PROC_API

So I'm going to attribute the patch to all three of us ;-)

>> @@ -483,7 +483,7 @@ create_procinfo (int pid, int tid)
>>      }
>>    else
>>      {
>> -      sprintf (pi->pathname, "/proc/%05d/lwp/%d", pid, tid);
>> +      sprintf (pi->pathname, "/proc/%d/lwp/%d", pid, tid);
>
> I am wondering how this ever worked for processes whose pid had
> fewer than 5 digits. I was initially concerned that this patch
> introduced a change of behavior that would create an incompatibility.
> But looking at Solaris 2.8 and 2.11 systems, I see processes with
> 3 or 4 digits PIDs, and the path in /proc doesn't have leading zeroes.

Indeed, and Solaris procfs doesn't care if the <pid> part contains
additional leading zeros or not.

> I also checked whether the file might be used on platforms other than
> Solaris (see configure.nat), and this does not appear to be the case.

True: I removed support for all other previous users when getting rid of
!NEW_PROC_API, IRIX and Tru64 UNIX support that had long been obsoleted.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Fix /proc pathname sizes on Solaris

[Joel Brobecker]

> I've done some more digging myself: here's what I found:
> 
> * The MAX_PROC_NAME_SIZE part (done slightly differently) was originally
>   done by Stefan Teleman when he imported gdb 7.6 into the userland repo
>   (gdb.procfs.c.patch).
> 
> * The change to create_procinfo originated with April Chin when
>   importing gdb 7.12.1 later (001-fix-proc-name-size.patch).
> 
> * I had to make minor adjustments for master to account for my removal
>   of !NEW_PROC_API
> 
> So I'm going to attribute the patch to all three of us ;-)

Sounds good :).

> >> @@ -483,7 +483,7 @@ create_procinfo (int pid, int tid)
> >>      }
> >>    else
> >>      {
> >> -      sprintf (pi->pathname, "/proc/%05d/lwp/%d", pid, tid);
> >> +      sprintf (pi->pathname, "/proc/%d/lwp/%d", pid, tid);
> >
> > I am wondering how this ever worked for processes whose pid had
> > fewer than 5 digits. I was initially concerned that this patch
> > introduced a change of behavior that would create an incompatibility.
> > But looking at Solaris 2.8 and 2.11 systems, I see processes with
> > 3 or 4 digits PIDs, and the path in /proc doesn't have leading zeroes.
> 
> Indeed, and Solaris procfs doesn't care if the <pid> part contains
> additional leading zeros or not.
> 
> > I also checked whether the file might be used on platforms other than
> > Solaris (see configure.nat), and this does not appear to be the case.
> 
> True: I removed support for all other previous users when getting rid of
> !NEW_PROC_API, IRIX and Tru64 UNIX support that had long been obsoleted.

Excellent. An extra confirmation is always nice.

-- 
Joel

Re: [PATCH] Fix /proc pathname sizes on Solaris

[Tom Tromey]

>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:

Rainer> -      sprintf (pi->pathname, "/proc/%05d/lwp/%d", pid, tid);
Rainer> +      sprintf (pi->pathname, "/proc/%d/lwp/%d", pid, tid);

FWIW gdb has xsnprintf which will check the length.  Or I suppose it
wouldn't be hard to just change this to be a std::string and not worry
about lengths.

Tom

Re: [PATCH] Fix /proc pathname sizes on Solaris

[Rainer Orth]

Hi Tom,

>>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
>
> Rainer> -      sprintf (pi->pathname, "/proc/%05d/lwp/%d", pid, tid);
> Rainer> +      sprintf (pi->pathname, "/proc/%d/lwp/%d", pid, tid);
>
> FWIW gdb has xsnprintf which will check the length.  Or I suppose it
> wouldn't be hard to just change this to be a std::string and not worry
> about lengths.

ah, good point.  I'd forgotten about that here, but it's already fixed
in a largish procfs.c cleanup patch which gets rids of tons of ARI
warnings among others.  Currently retesting...

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Fix /proc pathname sizes on Solaris

[Pedro Alves]

On 09/17/2018 03:11 PM, Rainer Orth wrote:
> * AFAIK Oracle has a corporate copyright assignment on file, so the
>   patches should be covered.  

It is my understanding that an assignment alone is not sufficient.
The patches should be willfully contributed somehow too.  The most
common form is by the author or representative contributing or
disclaiming it on the list of course.

> Even if that were not the case, this one
>   and the next are certainly below the 15-line limit for non-trivial
>   patches.

That certainly helps.

Thanks,
Pedro Alves

Re: [PATCH] Fix /proc pathname sizes on Solaris

[Rainer Orth]

Hi Pedro,

> On 09/17/2018 03:11 PM, Rainer Orth wrote:
>> * AFAIK Oracle has a corporate copyright assignment on file, so the
>>   patches should be covered.  
>
> It is my understanding that an assignment alone is not sufficient.
> The patches should be willfully contributed somehow too.  The most
> common form is by the author or representative contributing or
> disclaiming it on the list of course.

I wasn't aware of that.  In case any of the remaining patches from the
solaris-userland repo prove fit for submission, I'll check how this can
be done, given that the authors are no longer with Oracle.

>> Even if that were not the case, this one
>>   and the next are certainly below the 15-line limit for non-trivial
>>   patches.
>
> That certainly helps.

On top of that, both patches clearly document the intent already:

$ head 001-fix-proc-name-size.patch 
#
# Fix the size of the pathname for /proc files.
# In Solaris, PID_MAX is 999999 (6 digit pid).
# In Solaris, lwpid_t is an unsigned int, so theoretically the lwp id
# could be 10 digits.
# Patch will be submitted upstream.

$ head 007-solib-svr4.patch 
# Patch required for Solaris.
# Will contribute upstream if possible.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University