re: CVS commit: src/lib/libc/stdlib

[matthew green]

> Minor changes to jemalloc100 (the old one that only vax etc currently uses).

thanks.

i'm still using this version on a bunch of modern machines.

new jemalloc was problematic for a few things for me a
number of years ago and i keep meaning to test again, but
for now i'm still mostly using this version everwhere.

FYI.


.mrg.

Re: CVS commit: src/lib/libc/stdlib

[Robert Elz]

Date:Thu, 29 Sep 2022 08:18:49 -0400
From:Christos Zoulas 
Message-ID:  

  | Yes, I had forgotten about the need to do this explicitly...

  | > On Sep 28, 2022, at 10:23 PM, Robert Elz  wrote:
  | > 
  | > Apologies, I did not read the code closely enough, there must be a bug
  | > in the way the clone file descriptor is created.

I think I know the cause now, and it isn't anything specific to /dev/ptmx
it would affect all cloning devices.

In kern/vfs_syscalls.c do_open() we see the following:

if (vp == NULL) {
fd_abort(p, fp, indx);
error = fd_dupopen(dupfd, dupfd_move, flags, );
if (error)
return error;
*fd = indx;   
} else {
error = open_setfp(l, fp, vp, indx, flags);
if (error)
return error;
VOP_UNLOCK(vp);
*fd = indx;
fd_affix(p, fp, indx);
}

The vp==NULL case is where cloning devices are handled.   fd_dupopen()
arranges top make the duplicate happen.

It attempts to handle O_CLOEXEC thus:

error = fd_dup(fp, 0, newp, ff->ff_exclose);

where ff_exclose is the close_on_exec bit for the old file descriptor
(the one being duplicated) which is just fine for other calls of fd_dupopen().

The vp!=NULL case above is for "normal" opens, and open_setfp() is what
handles O_CLOEXEC - that's what (eventually) makes ff_exclose be true
(it also handles O_EXLOCK and O_SHLOCK.

None of that is being done in the cloning device open case, so those 3
flags simply cannot work, as the code is written currently, for these
devices.

I'm not about to go playing in this very fiddly piece of code, but I'm
kind of hoping that dholland@ might know the right magic sequence to make
this issue go away.

It probably means a call to open_setfp() with some parameters or other,
somewhere in the vp==NULL side of that if statement, though, just what
I have not attempted to work out.

kre

Re: CVS commit: src/lib/libc/stdlib

[Christos Zoulas]

Yes, I had forgotten about the need to do this explicitly...

christos

> On Sep 28, 2022, at 10:23 PM, Robert Elz  wrote:
> 
> Apologies, I did not read the code closely enough, there must be a bug
> in the way the clone file descriptor is created.
> 
> kre



signature.asc
Description: Message signed with OpenPGP

Re: CVS commit: src/lib/libc/stdlib

[Robert Elz]

Apologies, I did not read the code closely enough, there must be a bug
in the way the clone file descriptor is created.

kre

Re: CVS commit: src/lib/libc/stdlib

[Robert Elz]

Date:Wed, 28 Sep 2022 20:48:53 -0400
From:"David H. Gutteridge" 
Message-ID:  <9c9c8e9d9384338320b47dabfdc94...@gutteridge.ca>

   
  | (O_CLOEXEC, on the other hand, is ignored, at the moment.)

No it isn't, your test is faulty, O_CLOEXEC is a different
kind of flag, applies at a different level, and is fetched
a different way.

That's what dholland@ tried to tell you a few days ago.

kre
  |
  | $ cat open_test.c
  | #include 
  | #include 
  | #include 
  |
  | int main(int argc, char* argv[])
  | {
  | int fd, flags;
  |
  | printf("Regular file (read-only) with O_NONBLOCK | O_CLOEXEC.\n");
  | if ((fd = open("/etc/release", O_RDONLY | O_NONBLOCK | O_CLOEXEC)) < 0)
  | printf("Failed to get file handle.\n");
  | else {
  | printf("Descriptor flags: %d\n", flags = fcntl(fd, F_GETFD));
  | printf("Status flags: %d\n", flags = fcntl(fd, F_GETFL, 0));
  | close(fd);
  | }
  |
  | printf("POSIX pt cloning device with O_NONBLOCK | O_CLOEXEC.\n");
  | if ((fd = open("/dev/ptmx", O_RDWR | O_NOCTTY | O_NONBLOCK | 
  | O_CLOEXEC)) < 0)
  | printf("Failed to open /dev/ptmx.\n");
  | else {
  | printf("Descriptor flags: %d\n", flags = fcntl(fd, F_GETFD));
  | printf("Status flags: %d\n", flags = fcntl(fd, F_GETFL, 0));
  | close(fd);
  | }
  |
  | return 0;
  | }
  |
  | $ ./open_test
  | Regular file (read-only) with O_NONBLOCK | O_CLOEXEC.
  | Descriptor flags: 1
  | Status flags: 4
  | POSIX pt cloning device with O_NONBLOCK | O_CLOEXEC.
  | Descriptor flags: 0
  | Status flags: 6
  |
  | Regards,
  |
  | Dave
  |

Re: CVS commit: src/lib/libc/stdlib

[David H. Gutteridge]

On Wed, 28 Sep 2022, at 15:07:41 - (UTC), Christos Zoulas wrote:

In article <20220928003547.D2375FA92%cvs.NetBSD.org@localhost>,
David H. Gutteridge  wrote:

-=-=-=-=-=-

Module Name:src
Committed By:   gutteridge
Date:   Wed Sep 28 00:35:47 UTC 2022

Modified Files:
src/lib/libc/stdlib: posix_openpt.3

Log Message:
posix_openpt.3: reflect flag changes from r. 1.44 of tty_ptm.c

Some flags are now accepted, others are still ignored. (E.g., other
BSDs would return EINVAL if O_RDWR wasn't passed, and we now accept
O_NONBLOCK but not O_CLOEXEC.)


How so?

#define FCNTLFLAGS  
(FAPPEND|FASYNC|FFSYNC|FNONBLOCK|FDSYNC|FRSYNC|FALTIO|\

  ^
FDIRECT|FNOSIGPIPE)
/* bits to save after open(2) */
#define FMASK   (FREAD|FWRITE|FCNTLFLAGS|FEXEC)
 ^^

Best,

christos


Hi Christos,

I'm sorry, I don't follow your question? I wrote "we now accept
O_NONBLOCK...", which is what I'm reading you're saying too?

(O_CLOEXEC, on the other hand, is ignored, at the moment.)

$ cat open_test.c
#include 
#include 
#include 

int main(int argc, char* argv[])
{
int fd, flags;

printf("Regular file (read-only) with O_NONBLOCK | O_CLOEXEC.\n");
if ((fd = open("/etc/release", O_RDONLY | O_NONBLOCK | O_CLOEXEC)) < 0)
printf("Failed to get file handle.\n");
else {
printf("Descriptor flags: %d\n", flags = fcntl(fd, F_GETFD));
printf("Status flags: %d\n", flags = fcntl(fd, F_GETFL, 0));
close(fd);
}

printf("POSIX pt cloning device with O_NONBLOCK | O_CLOEXEC.\n");
	if ((fd = open("/dev/ptmx", O_RDWR | O_NOCTTY | O_NONBLOCK | 
O_CLOEXEC)) < 0)

printf("Failed to open /dev/ptmx.\n");
else {
printf("Descriptor flags: %d\n", flags = fcntl(fd, F_GETFD));
printf("Status flags: %d\n", flags = fcntl(fd, F_GETFL, 0));
close(fd);
}

return 0;
}

$ ./open_test
Regular file (read-only) with O_NONBLOCK | O_CLOEXEC.
Descriptor flags: 1
Status flags: 4
POSIX pt cloning device with O_NONBLOCK | O_CLOEXEC.
Descriptor flags: 0
Status flags: 6

Regards,

Dave

Re: CVS commit: src/lib/libc/stdlib

[Christos Zoulas]

In article <20220928003547.d2375f...@cvs.netbsd.org>,
David H. Gutteridge  wrote:
>-=-=-=-=-=-
>
>Module Name:   src
>Committed By:  gutteridge
>Date:  Wed Sep 28 00:35:47 UTC 2022
>
>Modified Files:
>   src/lib/libc/stdlib: posix_openpt.3
>
>Log Message:
>posix_openpt.3: reflect flag changes from r. 1.44 of tty_ptm.c
>
>Some flags are now accepted, others are still ignored. (E.g., other
>BSDs would return EINVAL if O_RDWR wasn't passed, and we now accept
>O_NONBLOCK but not O_CLOEXEC.)

How so?

#define FCNTLFLAGS  (FAPPEND|FASYNC|FFSYNC|FNONBLOCK|FDSYNC|FRSYNC|FALTIO|\
   ^
 FDIRECT|FNOSIGPIPE)
/* bits to save after open(2) */
#define FMASK   (FREAD|FWRITE|FCNTLFLAGS|FEXEC)
  ^^

Best,

christos

Re: CVS commit: src/lib/libc/stdlib

[Tobias Nygren]

On Sat, 12 Mar 2022 08:26:01 +
Nia Alarie  wrote:

> Module Name:  src
> Committed By: nia
> Date: Sat Mar 12 08:26:01 UTC 2022
> 
> Modified Files:
>   src/lib/libc/stdlib: hcreate.c
> 
> Log Message:
> hcreate(3): use reallocarr instead of malloc(x * y)

Caution: malloc(0) and reallocarr(, 0, s) have different semantics
for the returned pointer value.

Re: CVS commit: src/lib/libc/stdlib

[Warner Losh]

On Sat, Feb 12, 2022, 11:52 AM Taylor R Campbell <
campbell+netbsd-source-change...@mumble.net> wrote:

> > Date: Sun, 13 Feb 2022 05:44:38 +1100
> > from: matthew green 
> >
> > "Roland Illig" writes:
> > > Module Name:src
> > > Committed By:   rillig
> > > Date:   Fri Feb 11 21:36:46 UTC 2022
> > >
> > > Modified Files:
> > > src/lib/libc/stdlib: getenv.c
> > >
> > > Log Message:
> > > libc/getenv: remove trailing whitespace
> > >
> > > No binary change.
> >
> > please avoid purely whitespace changes unless you're
> > also going to be modifying the code itself.
> >
> > (don't back out now.)
>
> I thought we were supposed to _avoid_ mixing whitespace changes and
> functional changes?
>
> (Obviously the solution is to just only ever commit code with perfect
> style to begin with so this is never an issue!  Speaking of which,
> (setq show-trailing-whitespace t) is helpful to avoid this kind of
> mistake up front.  git diff or git add -p will also flag this kind of
> mistake, if you're working in git.  /usr/share/misc/NetBSD.el is also
> helpful for other KNF style.)
>

I thought the rule was don't make purely whitespace changes UNLESS you plan
on making other changes to (or are fixing an oops you just made). If that's
the case, separate the two commits.

Warner

>

Re: CVS commit: src/lib/libc/stdlib

[Roland Illig]

Am 12.02.2022 um 20:10 schrieb Warner Losh:

I thought the rule was don't make purely whitespace changes UNLESS you
plan on making other changes to (or are fixing an oops you just made).
If that's the case, separate the two commits.


That sounds like a useful rule, I'll stick to it.

Roland

Re: CVS commit: src/lib/libc/stdlib

[Taylor R Campbell]

> Date: Sun, 13 Feb 2022 05:44:38 +1100
> from: matthew green 
> 
> "Roland Illig" writes:
> > Module Name:src
> > Committed By:   rillig
> > Date:   Fri Feb 11 21:36:46 UTC 2022
> >
> > Modified Files:
> > src/lib/libc/stdlib: getenv.c
> >
> > Log Message:
> > libc/getenv: remove trailing whitespace
> >
> > No binary change.
> 
> please avoid purely whitespace changes unless you're
> also going to be modifying the code itself.
> 
> (don't back out now.)

I thought we were supposed to _avoid_ mixing whitespace changes and
functional changes?

(Obviously the solution is to just only ever commit code with perfect
style to begin with so this is never an issue!  Speaking of which,
(setq show-trailing-whitespace t) is helpful to avoid this kind of
mistake up front.  git diff or git add -p will also flag this kind of
mistake, if you're working in git.  /usr/share/misc/NetBSD.el is also
helpful for other KNF style.)

re: CVS commit: src/lib/libc/stdlib

[matthew green]

"Roland Illig" writes:
> Module Name:  src
> Committed By: rillig
> Date: Fri Feb 11 21:36:46 UTC 2022
>
> Modified Files:
>   src/lib/libc/stdlib: getenv.c
>
> Log Message:
> libc/getenv: remove trailing whitespace
>
> No binary change.

please avoid purely whitespace changes unless you're
also going to be modifying the code itself.

(don't back out now.)

thanks.


.mrg.

Re: CVS commit: src/lib/libc/stdlib

[Valery Ushakov]

On Sun, Feb 23, 2020 at 10:57:28 +0100, Kamil Rytarowski wrote:

> On 23.02.2020 08:46, Martin Husemann wrote:
> 
> > Source code consistency is a very important stylistic plus, every break of
> > that should be accompanied by a comment.
>
> Done.

Thank you.

-uwe

Re: CVS commit: src/lib/libc/stdlib

[Kamil Rytarowski]

On 23.02.2020 08:46, Martin Husemann wrote:
> On Sun, Feb 23, 2020 at 03:35:19AM +0100, Kamil Rytarowski wrote:
>> Algorithm would be changed from calculating on 32bit numbers with signed
>> integer overflows to an algorithm calculating on 64bit numbers. The
>> __dorand48() function truncates the result to least significant 16bits
>> only so it does not matter. I retained operations on 32bits avoiding
>> changes of types for stylistic reasons.
> 
> I am with uwe here - either it would not make any difference at all (on
> 32bit architectures) or it would end up with the same results and would
> make no performance difference (on 64 bit architectures), so going with
> the consistent (unsigned long) would have been fine.
> 
> Even better would be a cleanup to make it (uint32_t) everwhere, but of
> course only after carefull examination.
> 
> Source code consistency is a very important stylistic plus, every break of
> that should be accompanied by a comment.
> 
> Martin
> 

Done.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/lib/libc/stdlib

[Martin Husemann]

On Sun, Feb 23, 2020 at 03:35:19AM +0100, Kamil Rytarowski wrote:
> Algorithm would be changed from calculating on 32bit numbers with signed
> integer overflows to an algorithm calculating on 64bit numbers. The
> __dorand48() function truncates the result to least significant 16bits
> only so it does not matter. I retained operations on 32bits avoiding
> changes of types for stylistic reasons.

I am with uwe here - either it would not make any difference at all (on
32bit architectures) or it would end up with the same results and would
make no performance difference (on 64 bit architectures), so going with
the consistent (unsigned long) would have been fine.

Even better would be a cleanup to make it (uint32_t) everwhere, but of
course only after carefull examination.

Source code consistency is a very important stylistic plus, every break of
that should be accompanied by a comment.

Martin

Re: CVS commit: src/lib/libc/stdlib

[Valery Ushakov]

On Sun, Feb 23, 2020 at 03:35:19 +0100, Kamil Rytarowski wrote:

> On 23.02.2020 03:20, Valery Ushakov wrote:
> > On Sun, Feb 23, 2020 at 02:51:49 +0100, Kamil Rytarowski wrote:
> > 
> >> On 23.02.2020 02:29, Valery Ushakov wrote:
> >>> On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote:
> >>>
>  Module Name: src
>  Committed By:kamil
>  Date:Sat Feb 22 14:07:57 UTC 2020
> 
>  Modified Files:
>   src/lib/libc/stdlib: _rand48.c
> 
>  Log Message:
>  Avoid undefined behavior in the rand48(3) implementation
> 
>  Instead of implicid promotion to signed int,
>  explicitly cast the arguments to unsigned int.
> >>>
> >>> Please, please, please, pay at least some attention to what is going
> >>> on around the code you are changing.
> >>>
> >>> If there's already code in this function that does:
> >>>
> >>>accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0];
> >>>
> >>> then keep it consistent and don't do casts to a different type
> >>>
> >>>accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2];
> >>
> >> cast to unsigned long still works, but changes algorithm. My change was
> >> performed deliberately. On the other hand and according to local tests
> >> the end-result for unsigned long produces the same reults as cast to
> >> unsigned int and unsigned char so it does not matter.
> > 
> > I cannot make sense of your answer.  Does the cast to unsigned long
> > there change the algorithm or does it produce the same result?  If it
> > produces the same result, then it should be used to be consistent with
> > the rest of the code (or the rest of the code changed to use unsigned
> > int).  If it does change the result, there should be a comment
> > explaining it.
> 
> Algorithm would be changed from calculating on 32bit numbers with signed
> integer overflows to an algorithm calculating on 64bit numbers. The
> __dorand48() function truncates the result to least significant 16bits
> only so it does not matter. I retained operations on 32bits avoiding
> changes of types for stylistic reasons.

That still doesn't make sense to me.  You took your time to figure out
whats going on in this bit of code.  Then you make a change that looks
extremely unobvious b/c it is inconsistent with the rest of the code,
and you say you did this for stylistic reasons.

The next person looking at that code (in $bignum years) will have to
waste their time puzzling out the reason.  Why not use the knowledge
you've gained of this code for good and change the code properly?  The
90s unsigned long was probably meant to be 32-bit anyway (cf. X11 mess
up with using long for 32 bit quantities in the protocol and then
running into issues when 64-bit happened).  So if doing things in
32-bit here is the right and intended thing to do, then change that
unsigned long to uint32_t.  If you don't want to dive that deep (which
is entirely understandable), then, exactly for stylistic reasons, cast
to unsigned long to be consistent with the old code - as you already
established that it didn't change the result.

-uwe

Re: CVS commit: src/lib/libc/stdlib

[Kamil Rytarowski]

On 23.02.2020 03:20, Valery Ushakov wrote:
> On Sun, Feb 23, 2020 at 02:51:49 +0100, Kamil Rytarowski wrote:
> 
>> On 23.02.2020 02:29, Valery Ushakov wrote:
>>> On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote:
>>>
 Module Name:   src
 Committed By:  kamil
 Date:  Sat Feb 22 14:07:57 UTC 2020

 Modified Files:
src/lib/libc/stdlib: _rand48.c

 Log Message:
 Avoid undefined behavior in the rand48(3) implementation

 Instead of implicid promotion to signed int,
 explicitly cast the arguments to unsigned int.
>>>
>>> Please, please, please, pay at least some attention to what is going
>>> on around the code you are changing.
>>>
>>> If there's already code in this function that does:
>>>
>>>accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0];
>>>
>>> then keep it consistent and don't do casts to a different type
>>>
>>>accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2];
>>
>> cast to unsigned long still works, but changes algorithm. My change was
>> performed deliberately. On the other hand and according to local tests
>> the end-result for unsigned long produces the same reults as cast to
>> unsigned int and unsigned char so it does not matter.
> 
> I cannot make sense of your answer.  Does the cast to unsigned long
> there change the algorithm or does it produce the same result?  If it
> produces the same result, then it should be used to be consistent with
> the rest of the code (or the rest of the code changed to use unsigned
> int).  If it does change the result, there should be a comment
> explaining it.
> 
> -uwe
> 

Algorithm would be changed from calculating on 32bit numbers with signed
integer overflows to an algorithm calculating on 64bit numbers. The
__dorand48() function truncates the result to least significant 16bits
only so it does not matter. I retained operations on 32bits avoiding
changes of types for stylistic reasons.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/lib/libc/stdlib

[Valery Ushakov]

On Sun, Feb 23, 2020 at 02:51:49 +0100, Kamil Rytarowski wrote:

> On 23.02.2020 02:29, Valery Ushakov wrote:
> > On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote:
> > 
> >> Module Name:   src
> >> Committed By:  kamil
> >> Date:  Sat Feb 22 14:07:57 UTC 2020
> >>
> >> Modified Files:
> >>src/lib/libc/stdlib: _rand48.c
> >>
> >> Log Message:
> >> Avoid undefined behavior in the rand48(3) implementation
> >>
> >> Instead of implicid promotion to signed int,
> >> explicitly cast the arguments to unsigned int.
> > 
> > Please, please, please, pay at least some attention to what is going
> > on around the code you are changing.
> > 
> > If there's already code in this function that does:
> > 
> >accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0];
> > 
> > then keep it consistent and don't do casts to a different type
> > 
> >accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2];
> 
> cast to unsigned long still works, but changes algorithm. My change was
> performed deliberately. On the other hand and according to local tests
> the end-result for unsigned long produces the same reults as cast to
> unsigned int and unsigned char so it does not matter.

I cannot make sense of your answer.  Does the cast to unsigned long
there change the algorithm or does it produce the same result?  If it
produces the same result, then it should be used to be consistent with
the rest of the code (or the rest of the code changed to use unsigned
int).  If it does change the result, there should be a comment
explaining it.

-uwe

Re: CVS commit: src/lib/libc/stdlib

[Kamil Rytarowski]

On 23.02.2020 02:29, Valery Ushakov wrote:
> On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote:
> 
>> Module Name: src
>> Committed By:kamil
>> Date:Sat Feb 22 14:07:57 UTC 2020
>>
>> Modified Files:
>>  src/lib/libc/stdlib: _rand48.c
>>
>> Log Message:
>> Avoid undefined behavior in the rand48(3) implementation
>>
>> Instead of implicid promotion to signed int,
>> explicitly cast the arguments to unsigned int.
> 
> Please, please, please, pay at least some attention to what is going
> on around the code you are changing.
> 
> If there's already code in this function that does:
> 
>accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0];
> 
> then keep it consistent and don't do casts to a different type
> 
>accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2];
> 
> 
> -uwe
> 

cast to unsigned long still works, but changes algorithm. My change was
performed deliberately. On the other hand and according to local tests
the end-result for unsigned long produces the same reults as cast to
unsigned int and unsigned char so it does not matter.



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/lib/libc/stdlib

[Valery Ushakov]

On Sat, Feb 22, 2020 at 14:07:57 +, Kamil Rytarowski wrote:

> Module Name:  src
> Committed By: kamil
> Date: Sat Feb 22 14:07:57 UTC 2020
> 
> Modified Files:
>   src/lib/libc/stdlib: _rand48.c
> 
> Log Message:
> Avoid undefined behavior in the rand48(3) implementation
> 
> Instead of implicid promotion to signed int,
> explicitly cast the arguments to unsigned int.

Please, please, please, pay at least some attention to what is going
on around the code you are changing.

If there's already code in this function that does:

   accu = (unsigned long) __rand48_mult[0] * (unsigned long) xseed[0];

then keep it consistent and don't do casts to a different type

   accu += (unsigned int) __rand48_mult[0] * (unsigned int) xseed[2];


-uwe

Re: CVS commit: src/lib/libc/stdlib

[Joerg Sonnenberger]

On Thu, Nov 02, 2017 at 08:26:58PM +0100, Kamil Rytarowski wrote:
> On 02.11.2017 20:15, Joerg Sonnenberger wrote:
> > On Thu, Nov 02, 2017 at 08:15:01PM +0100, Joerg Sonnenberger wrote:
> >> On Thu, Nov 02, 2017 at 06:37:15PM +, Kamil Rytarowski wrote:
> >>> Module Name:  src
> >>> Committed By: kamil
> >>> Date: Thu Nov  2 18:37:15 UTC 2017
> >>>
> >>> Modified Files:
> >>>   src/lib/libc/stdlib: atexit.c
> >>>
> >>> Log Message:
> >>> Correct handling of __cxa_atexit(a,b,NULL) in libc
> >>
> >> I don't get it. This seems to be quite wrong.
> >>
> >>> Correct a bug that __cxa_atexit(x,y,NULL) is handled in the same way as
> >>> atexit(x) (which is simplified to __cxa_atexit(x,NULL,NULL).
> >>
> >> __cxa_atexit(x,y,NULL) is not invalid. Please revert this.
> > 
> > ...is invalid, of course.
> > 
> > Joerg
> > 
> 
> What's the rationale for being wrong?

The DSO handle is a required part of the (external) __cxa_atexit interface.
The atexit mapping is an implementation detail and not part of the public
interface. Doing it directly creates UB as it involves casting function
pointers between incompatible types.

Joerg

Re: CVS commit: src/lib/libc/stdlib

[Kamil Rytarowski]

On 02.11.2017 20:15, Joerg Sonnenberger wrote:
> On Thu, Nov 02, 2017 at 08:15:01PM +0100, Joerg Sonnenberger wrote:
>> On Thu, Nov 02, 2017 at 06:37:15PM +, Kamil Rytarowski wrote:
>>> Module Name:src
>>> Committed By:   kamil
>>> Date:   Thu Nov  2 18:37:15 UTC 2017
>>>
>>> Modified Files:
>>> src/lib/libc/stdlib: atexit.c
>>>
>>> Log Message:
>>> Correct handling of __cxa_atexit(a,b,NULL) in libc
>>
>> I don't get it. This seems to be quite wrong.
>>
>>> Correct a bug that __cxa_atexit(x,y,NULL) is handled in the same way as
>>> atexit(x) (which is simplified to __cxa_atexit(x,NULL,NULL).
>>
>> __cxa_atexit(x,y,NULL) is not invalid. Please revert this.
> 
> ...is invalid, of course.
> 
> Joerg
> 

What's the rationale for being wrong?



signature.asc
Description: OpenPGP digital signature

Re: CVS commit: src/lib/libc/stdlib

[Joerg Sonnenberger]

On Thu, Nov 02, 2017 at 08:15:01PM +0100, Joerg Sonnenberger wrote:
> On Thu, Nov 02, 2017 at 06:37:15PM +, Kamil Rytarowski wrote:
> > Module Name:src
> > Committed By:   kamil
> > Date:   Thu Nov  2 18:37:15 UTC 2017
> > 
> > Modified Files:
> > src/lib/libc/stdlib: atexit.c
> > 
> > Log Message:
> > Correct handling of __cxa_atexit(a,b,NULL) in libc
> 
> I don't get it. This seems to be quite wrong.
> 
> > Correct a bug that __cxa_atexit(x,y,NULL) is handled in the same way as
> > atexit(x) (which is simplified to __cxa_atexit(x,NULL,NULL).
> 
> __cxa_atexit(x,y,NULL) is not invalid. Please revert this.

...is invalid, of course.

Joerg

Re: CVS commit: src/lib/libc/stdlib

[Joerg Sonnenberger]

On Thu, Nov 02, 2017 at 06:37:15PM +, Kamil Rytarowski wrote:
> Module Name:  src
> Committed By: kamil
> Date: Thu Nov  2 18:37:15 UTC 2017
> 
> Modified Files:
>   src/lib/libc/stdlib: atexit.c
> 
> Log Message:
> Correct handling of __cxa_atexit(a,b,NULL) in libc

I don't get it. This seems to be quite wrong.

> Correct a bug that __cxa_atexit(x,y,NULL) is handled in the same way as
> atexit(x) (which is simplified to __cxa_atexit(x,NULL,NULL).

__cxa_atexit(x,y,NULL) is not invalid. Please revert this.

Joerg

Re: CVS commit: src/lib/libc/stdlib

[Christos Zoulas]

In article <20170112213155.ga11...@britannica.bec.de>,
Joerg Sonnenberger   wrote:
>On Wed, Jan 11, 2017 at 09:00:42PM -0500, Christos Zoulas wrote:
>> Module Name: src
>> Committed By:christos
>> Date:Thu Jan 12 02:00:42 UTC 2017
>> 
>> Modified Files:
>>  src/lib/libc/stdlib: malloc.c
>> 
>> Log Message:
>> Avoid sysconf: __sysconf -> sysctlgetmibinfo -> strtoimax -> locale, etc.
>
>Can you at least use sysctl(2) directly?

I could... Ok.

christos

Re: CVS commit: src/lib/libc/stdlib

[Joerg Sonnenberger]

On Wed, Jan 11, 2017 at 09:00:42PM -0500, Christos Zoulas wrote:
> Module Name:  src
> Committed By: christos
> Date: Thu Jan 12 02:00:42 UTC 2017
> 
> Modified Files:
>   src/lib/libc/stdlib: malloc.c
> 
> Log Message:
> Avoid sysconf: __sysconf -> sysctlgetmibinfo -> strtoimax -> locale, etc.

Can you at least use sysctl(2) directly?

Joerg

Re: CVS commit: src/lib/libc/stdlib

[Joerg Sonnenberger]

On Sun, Aug 16, 2015 at 11:07:24PM +0200, Kamil Rytarowski wrote:
 What do you think about the following patch:

No.

 In 90. division was an expensive operation, today not any more. I
 would prefer to let it to be optimized by a compiler, not by a human
 for a special or an old hardware with expensive arithmetic operations.

That's plainly wrong. A division is still slow and horribly slow on
quite a few CPUs we care about.

Joerg

Re: CVS commit: src/lib/libc/stdlib

[Kamil Rytarowski]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 09.08.2015 15:29, Joerg Sonnenberger wrote:
 On Tue, Jul 28, 2015 at 05:13:34PM +, Kamil Rytarowski wrote:
 Module Name: src Committed By:   kamil Date: Tue Jul 28 
 17:13:34
 UTC 2015
 
 Modified Files: src/lib/libc/stdlib: reallocarr.3 reallocarr.c
 
 Log Message: Compatibility fixes in reallocarr(3)
 
 Except it is worse now. There is now an obscure assumption that 
 CHAR_BITS is 8, spelled via 2 == log_2 CHAR_BITS - 1.
 


What do you think about the following patch:

Index: reallocarr.c
===
RCS file: /cvsroot/src/lib/libc/stdlib/reallocarr.c,v
retrieving revision 1.3
diff -u -r1.3 reallocarr.c
- --- reallocarr.c  28 Jul 2015 17:13:34 -  1.3
+++ reallocarr.c16 Aug 2015 20:57:06 -
@@ -50,8 +50,6 @@
 #endif
 #endif

- -#define SQRT_SIZE_MAX (1UL  (sizeof(size_t)  2))
- -
 #if !HAVE_REALLOCARR
 int
 reallocarr(void *ptr, size_t number, size_t size)
@@ -70,8 +68,7 @@
return 0;
}

- - if ((number = SQRT_SIZE_MAX || size = SQRT_SIZE_MAX) 
- - number  SIZE_MAX / size) {
+   if (number  SIZE_MAX / size) {
errno = saved_errno;
return EOVERFLOW;
}



I was benchmarking it on a modern amd64 processor and there is almost
no difference between (number = SQRT_SIZE_MAX || size =
SQRT_SIZE_MAX) vs (number  SIZE_MAX / size) for small 'number' and
small 'size'.

The reason to go for it is already prevented the 0 division with the
check in the free(3) case (number == 0 || size == 0).

In 90. division was an expensive operation, today not any more. I
would prefer to let it to be optimized by a compiler, not by a human
for a special or an old hardware with expensive arithmetic operations.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJV0PuKAAoJEEuzCOmwLnZsLmoP/3xbcc2XNXhyGmP/aZvZ9FTo
9ScYE50pV96J+UPmf5hM2kG7jUV7nIfPc3vSdP1UUi7Lg5B5gc6KKx9gCqPOn3PC
4HOcCLRRcO9R2VwNuTYSp5h1xRGgrKXt8fE5FEUA5WrMQ5+LgZ9nWQc1ZnLZFd38
Hir3o1AcjjLcLC2Xttq0CD9Uw7i1F5nUPMKfVgYo4CzYbLrTB+oXhymLOzqEcc2Q
N9aqUsFhCVphz24hH7KQX/sSbAykl6mBvv3JIGrurk6mby4UC2H26lEx+6LZw2eu
6TylkXrAmgC64Y5MhJkYnWDtKuwN09fr+zc0GsGpy4ZIEWD7rz1VCaovhLw2g7iV
u0ccHqe6ldLUzneqEKOIQJFTQU4ROM6KVhzCOrg4tTZw5MPd12ARYJz9hkdT9Nz1
clkH9+YMFZpRW9OnAze3U1APevmhP0qt7mpiWiMn8I49t+2fpMg5gt63dAyGLODw
tHK8IsCwuDp9TcvOI2iza+XhYpbBcjRoevTTciOa6xKougp62h+mCpIWrXGECKIz
Gomx3nzmIVAAVPIOoZAvwzD00n2vlVjjYEtw8fZ4q7QJueSYghhW/WSe4N6ZQDxD
OaTY0eeEOYHm9yGfy/L8sd5+jU7ehRbBHJ5MtAkFzO07HTeNmlMWtV1PV7VfAByB
e4MfvasHmIj5l7QQVyAo
=4eOR
-END PGP SIGNATURE-

Re: CVS commit: src/lib/libc/stdlib

[Joerg Sonnenberger]

On Tue, Jul 28, 2015 at 05:13:34PM +, Kamil Rytarowski wrote:
 Module Name:  src
 Committed By: kamil
 Date: Tue Jul 28 17:13:34 UTC 2015
 
 Modified Files:
   src/lib/libc/stdlib: reallocarr.3 reallocarr.c
 
 Log Message:
 Compatibility fixes in reallocarr(3)

Except it is worse now. There is now an obscure assumption that
CHAR_BITS is 8, spelled via 2 == log_2 CHAR_BITS - 1.

 While there: rename parameter name 'num' to 'number' to be in sync with
 the calloc(3) parameter naming.

This change doesn't even make sense.

Joerg

Re: CVS commit: src/lib/libc/stdlib

[Joerg Sonnenberger]

On Sun, Jul 19, 2015 at 11:13:48PM +0200, Kamil Rytarowski wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA256
 
 On 19.07.2015 20:56, Joerg Sonnenberger wrote:
  On Thu, Jul 16, 2015 at 12:12:27PM +0200, Kamil Rytarowski wrote:
  The C standard permits memcpy(3) to affect errno(2).
  
  More like it hasn't explicitly ruled it out. That might or might 
  not be seen as an oversight, but pretty much any compiler uses 
  memcpy(3) under the assumption that it does not. If you really care
  about this case, please raise a DR against ISO C and get a 
  clarification of the intention, before further changes to src.
  
 
 Thanks.
 It is also for the consistency with lines 63-64 of the same file.

...which is quite different. free(3) is quite likely to set errno for a
number of reasons and historic versions at least have been known to
actually have failure modes.

Joerg

Re: CVS commit: src/lib/libc/stdlib

[Kamil Rytarowski]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 19.07.2015 20:56, Joerg Sonnenberger wrote:
 On Thu, Jul 16, 2015 at 12:12:27PM +0200, Kamil Rytarowski wrote:
 The C standard permits memcpy(3) to affect errno(2).
 
 More like it hasn't explicitly ruled it out. That might or might 
 not be seen as an oversight, but pretty much any compiler uses 
 memcpy(3) under the assumption that it does not. If you really care
 about this case, please raise a DR against ISO C and get a 
 clarification of the intention, before further changes to src.
 

Thanks.
It is also for the consistency with lines 63-64 of the same file.

Regards,
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJVrBMIAAoJEEuzCOmwLnZsvUsQAIMgD3/aL6pld6VZ521n0+kY
zsv3axqXVO4JwoEYj0g/B4W6vuuWBzkKrSVLJm6Hf/Xjv0ZfpvWwFbT9cq3p5zuH
3uYPhjxNuPM7HTSJvLaQUCK9X0kyB1DEVlF31UOqYE5Tg6zyH9I8SWWYH9uZx/kJ
2Tc3Fl1Vx/p9veyZTsQ0tddEokC97ABTo8tLkBrr7NTimi9G0WfgEMN9fcuMjzSt
BdsnEoYI/w3JZZOcE8HhmgAIJ3osX3Ys9RVmmKJzoIyQr5Sp61hrTsPBM4bx8n55
twbFygd7Fuo/Uc8muVhBDRRBkTLogwsDUz5htP0ydYy0PpLdGeZe1q+4yVUJwxMC
vQqwFDDk4lUvfhq/Rn3RPr8otE8/W9iltihkMF7BngDyWW+mQM4MjGmgfaesR/Kc
9CoIy1zylwD+Q9FcPQFNh0zMcxxtUQ0I4TFsMTaMZ6C2FzNWkuWp5rn2kvL/Io4c
xhiGZA+czOeEZBj9CNA3fWLAzA3R3UFWk66n8de8iWeWmWgBxv4d7LnHL5J4il/u
B/Pm5thHDHTBeSwsf2nAuCady6MXivxZJLSyAodXHDpWLNgBsOXCg7cTasXnCQOL
Oqeavut0qB6HeeaaOM2YhiO/eprE7kfA1UgxyXgysaBw23LiYAbaetKJ39K4A8L6
9mL4NLMc3uEvanz5goIZ
=Mayj
-END PGP SIGNATURE-

Re: CVS commit: src/lib/libc/stdlib

[Joerg Sonnenberger]

On Thu, Jul 16, 2015 at 12:12:27PM +0200, Kamil Rytarowski wrote:
 The C standard permits memcpy(3) to affect errno(2).

More like it hasn't explicitly ruled it out. That might or might not be
seen as an oversight, but pretty much any compiler uses memcpy(3) under
the assumption that it does not. If you really care about this case,
please raise a DR against ISO C and get a clarification of the
intention, before further changes to src.

Joerg

Re: CVS commit: src/lib/libc/stdlib

[Kamil Rytarowski]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On 16.07.2015 10:25, Pierre Pronchery wrote:
 Hi,
 
 On 07/16/15 02:04, Kamil Rytarowski wrote:
 Module Name: src Committed By:   kamil Date: Thu Jul 16 
 00:04:00
 UTC 2015
 
 Modified Files: src/lib/libc/stdlib: reallocarr.c
 
 Log Message: Reorder memcpy(3) and save errno
 
 This chang is for safety as memcpy(3) might change it.
 
 I am not against the change, but are you sure memcpy(3) has any
 chance to change errno? I think it would actually be a bug if it
 does, since it is not meant to fail, and it is a pretty essential
 component for error recovery (writing messages...).
 
 This is not a definite answer but: $ find
 NetBSD/src/common/lib/libc -name memcpy.S -exec grep errno {} \+ 
 does not report anything.
 
 Anyway, HTH,
 

I will use this function outside the NetBSD world. My intention is to
push it to libnbcompat. Before that I removed the possible
thin-skinned point.

The C standard permits memcpy(3) to affect errno(2).

I'm not much interested in debate what platforms do it and whether
they are sane or not, this change just cuts this discussion down. Last
year the GCC(1) team debated the support of platforms clobbering
errno(2) in the memcpy(9) function.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56888

Just better to do it now then wait for timer bomb to explode.

For most of the users this change is non-functional.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJVp4OIAAoJEEuzCOmwLnZstk8QALw6r4V+JDdJvi7ru4dqGWwm
5HjnM+YbmEjcbcunctZtPYXNLLl5c2KedBDaSmrZ6AJ192Xk7SCFR2jzZmQ/nO+G
xXp8yAp1bIqh/wM+FXtaeQYq7pc8z7ckvi9E7nreoife+kHC6fDuvhp9i6As5DoK
Vhqp9XaZfdCX7aWdkwlvf1gO7CpuBuvFNBD+9m80ganJ6Bwk/0Lv+hp+r2q6kQCC
9quq5GfAYwMpB0VVb2mtEVOo2wxBeFFk866lyiiZH6jf9PI8C2Igg7S2ljr463kv
p/DDgKWlY26FLkmagOaLyIGyQpni0Gw+XZg8qwAJ/H8UETls6XLLk5XPS5FbaoFc
xEgNVHYU3RxO/1eDyvtQGuKuoDfTFqIXVnsvBqhoGNBNSENwcgwc0AXcnWgbw/IY
CgIh80FUfrWvbnoxajgBGzVzOs4yjKpWcjbVrJl2tfjLCuQ/99EFptBuBBZWaquO
loqlqCO5MTWPeG3GUecUXBDZK6CJVf/Uz9II6YBSOXjTzIkGbVIev0beKMmXePEJ
KcxkStaFvVYIJ9Qbyzhd0/o03KZp2BM1tu1bIseHd1L9JNtleNLMSxb6WFDx69D2
ggmGgA9w4ySM7N/c2BEuKUurRRd4uElj2eJAnifSkaLhLg8184XK/0OFvLMVyFAJ
PUpnHTlLhZ9N7y3hn2/Z
=cZYP
-END PGP SIGNATURE-

Re: CVS commit: src/lib/libc/stdlib

[Pierre Pronchery]

Hi,

On 07/16/15 02:04, Kamil Rytarowski wrote:
 Module Name:  src
 Committed By: kamil
 Date: Thu Jul 16 00:04:00 UTC 2015
 
 Modified Files:
   src/lib/libc/stdlib: reallocarr.c
 
 Log Message:
 Reorder memcpy(3) and save errno
 
 This chang is for safety as memcpy(3) might change it.

I am not against the change, but are you sure memcpy(3) has any chance
to change errno? I think it would actually be a bug if it does, since it
is not meant to fail, and it is a pretty essential component for error
recovery (writing messages...).

This is not a definite answer but:
$ find NetBSD/src/common/lib/libc -name memcpy.S -exec grep errno {} \+
does not report anything.

Anyway, HTH,
-- 
khorben

Re: CVS commit: src/lib/libc/stdlib

[Izumi Tsutsui]

 Module Name:  src
 Committed By: christos
 Date: Wed Jan  8 02:15:42 UTC 2014
 
 Modified Files:
   src/lib/libc/stdlib: Makefile.inc ptsname.3 pty.c
 
 Log Message:
 add ptsname_r

Needs libc minor bump?

---
Izumi Tsutsui

Re: CVS commit: src/lib/libc/stdlib

[Christos Zoulas]

On Jan 8,  7:25pm, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/lib/libc/stdlib

|  Module Name:src
|  Committed By:   christos
|  Date:   Wed Jan  8 02:15:42 UTC 2014
|  
|  Modified Files:
|  src/lib/libc/stdlib: Makefile.inc ptsname.3 pty.c
|  
|  Log Message:
|  add ptsname_r
| 
| Needs libc minor bump?

I thought about it, but really ~nothing uses it.

christos

Re: CVS commit: src/lib/libc/stdlib

[Izumi Tsutsui]

 |  Module Name:  src
 |  Committed By: christos
 |  Date: Wed Jan  8 02:15:42 UTC 2014
 |  
 |  Modified Files:
 |src/lib/libc/stdlib: Makefile.inc ptsname.3 pty.c
 |  
 |  Log Message:
 |  add ptsname_r
 | 
 | Needs libc minor bump?
 
 I thought about it, but really ~nothing uses it.

Then why do you add it if it will be never used?

---
Izumi Tsutsui

re: CVS commit: src/lib/libc/stdlib

[matthew green]

 |  Module Name:  src
 |  Committed By: christos
 |  Date: Wed Jan  8 02:15:42 UTC 2014
 |  
 |  Modified Files:
 |src/lib/libc/stdlib: Makefile.inc ptsname.3 pty.c
 |  
 |  Log Message:
 |  add ptsname_r
 | 
 | Needs libc minor bump?
 
 I thought about it, but really ~nothing uses it.

i don't see why this matters.  libc grew a symbol?  it needs a
minor bump.


.rg.

Re: CVS commit: src/lib/libc/stdlib

[Christos Zoulas]

On Jan 8,  8:52pm, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: CVS commit: src/lib/libc/stdlib

| Then why do you add it if it will be never used?

It will be never used != is not currently being used.

I was reading the linux man page and it seemed easy to implement.

christos

Re: CVS commit: src/lib/libc/stdlib

[Izumi Tsutsui]

 On Jan 8,  8:52pm, tsut...@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
 -- Subject: Re: CVS commit: src/lib/libc/stdlib
 
 | Then why do you add it if it will be never used?
 
 It will be never used != is not currently being used.

Then you must bump minor otherwise future binaries will fail silently.

---
Izumi Tsutusi

Re: CVS commit: src/lib/libc/stdlib

[Alan Barrett]

On Tue, 20 Mar 2012, Christos Zoulas wrote:

Modified Files:
src/lib/libc/stdlib: jemalloc.c

-static char*umax2s(uintmax_t x, char *s);
+static char*umax2s(size_t x, char *s);


If you change the argument type, then please also change the 
function name.  Right now, the name contains the string umax 
which imnplies that the function deals with uintmax_t upjects, but 
the function actually deals with size_t opjects.


--apb (Alan Barrett)

Re: CVS commit: src/lib/libc/stdlib

[Christos Zoulas]

On May 17, 11:57am, tsugutomo.en...@jp.sony.com (tsugutomo.en...@jp.sony.com) 
wrote:
-- Subject: Re: CVS commit: src/lib/libc/stdlib

| Christos Zoulas chris...@netbsd.org writes:
| 
|  Module Name:src
|  Committed By:   christos
|  Date:   Fri May 13 23:11:00 UTC 2011
| 
|  Modified Files:
|  src/lib/libc/stdlib: jemalloc.c malloc.c
| 
|  Log Message:
|  don't let readlink trash errno.;
| 
| 
|  To generate a diff of this commit:
|  cvs rdiff -u -r1.22 -r1.23 src/lib/libc/stdlib/jemalloc.c
|  cvs rdiff -u -r1.52 -r1.53 src/lib/libc/stdlib/malloc.c
| 
| - Is the existing guard in malloc_init() of malloc.c insufficient?
| 
| - In the malloc_init_hard() of jemalloc.c, there are another calls may
| set errno.  Especially, the code explicitly handles an error from
| sysctl(3).

I will take a look, thanks. Which malloc are we using now? The problem
manifested itself by just calling strtod(1.0, NULL) in main and looking
at errno.

christos

Re: CVS commit: src/lib/libc/stdlib

[Christos Zoulas]

In article tkrr57yc9uf@gco-w12f-177-176.sm.sony.co.jp,
 tsugutomo.en...@jp.sony.com wrote:
Christos Zoulas chris...@netbsd.org writes:

 Module Name: src
 Committed By:christos
 Date:Fri May 13 23:11:00 UTC 2011

 Modified Files:
  src/lib/libc/stdlib: jemalloc.c malloc.c

 Log Message:
 don't let readlink trash errno.;


 To generate a diff of this commit:
 cvs rdiff -u -r1.22 -r1.23 src/lib/libc/stdlib/jemalloc.c
 cvs rdiff -u -r1.52 -r1.53 src/lib/libc/stdlib/malloc.c

- Is the existing guard in malloc_init() of malloc.c insufficient?

No that works; I undid it.

- In the malloc_init_hard() of jemalloc.c, there are another calls may
set errno.  Especially, the code explicitly handles an error from
sysctl(3).

I widened the scope, thanks.

christos

Re: CVS commit: src/lib/libc/stdlib

[tsugutomo . enami]

Christos Zoulas chris...@netbsd.org writes:

 Module Name:  src
 Committed By: christos
 Date: Fri May 13 23:11:00 UTC 2011

 Modified Files:
   src/lib/libc/stdlib: jemalloc.c malloc.c

 Log Message:
 don't let readlink trash errno.;


 To generate a diff of this commit:
 cvs rdiff -u -r1.22 -r1.23 src/lib/libc/stdlib/jemalloc.c
 cvs rdiff -u -r1.52 -r1.53 src/lib/libc/stdlib/malloc.c

- Is the existing guard in malloc_init() of malloc.c insufficient?

- In the malloc_init_hard() of jemalloc.c, there are another calls may
set errno.  Especially, the code explicitly handles an error from
sysctl(3).

enami.

Re: CVS commit: src/lib/libc/stdlib

[Alan Barrett]

On Mon, 01 Nov 2010, enami tsugutomo wrote:
 Modified Files:
   src/lib/libc/stdlib: getenv.c
 
 Log Message:
 Double the array only when really necessary.  Otherwise memory will be
 exhausted if user modifies the variable envrion itself repeatedly..

Now the code disagrees with the comment above it.  Please fix the comment.

   /* Make sure we at least double the size of the arrays. */
 - new_len = (environ_malloced_len = 16) ?
 - (environ_malloced_len  1) : 16;
 + new_len = environ_malloced_len = 16 ? environ_malloced_len : 16;

--apb (Alan Barrett)

Re: CVS commit: src/lib/libc/stdlib

[enami tsugutomo]

 Now the code disagrees with the comment above it.  Please fix the comment.

I've changed the comment but feel free to improve it if you have
better one.

enami.

Re: CVS commit: src/lib/libc/stdlib

[Matthias Scheler]

On Thu, Sep 30, 2010 at 03:05:26PM +0200, Nicolas Joly wrote:
 One possibility could be to not free memory at all in setenv, but only
 with unsetenv.

I'm not convinced about that.

 IMHO, it's a programmer error to call setenv more than once on the
 same variable without a corresponding unsetenv in between (just like
 malloc()/free() behaviour).

Is there any standard which backs this statement?

 Otherwise, if i understand things
 correctly, it's more putenv that shoud work that way.

Our putenv(3) is implemented via setenv(3).

Kind regards

-- 
Matthias Scheler  http://zhadum.org.uk/

Re: CVS commit: src/lib/libc/stdlib

[Nicolas Joly]

On Thu, Sep 30, 2010 at 02:11:19PM +0100, Matthias Scheler wrote:
 On Thu, Sep 30, 2010 at 03:05:26PM +0200, Nicolas Joly wrote:
  One possibility could be to not free memory at all in setenv, but only
  with unsetenv.
 
 I'm not convinced about that.
 
  IMHO, it's a programmer error to call setenv more than once on the
  same variable without a corresponding unsetenv in between (just like
  malloc()/free() behaviour).
 
 Is there any standard which backs this statement?

Not that i know.

  Otherwise, if i understand things
  correctly, it's more putenv that shoud work that way.
 
 Our putenv(3) is implemented via setenv(3).

That may be part of the problem (at least for zsh 4.2).

http://www.opengroup.org/onlinepubs/009695399/functions/putenv.html

From the OpenGroup function description; this function does not copy
the provided string, but use it directly instead. It's the caller
responsability to clean it when not in use anymore.

zsh 4.2 seems to follow this and try to deallocate the previous
variable string when it has been replaced by a new one. But our
putenv, which calls setenv, has already done the same ...

-- 
Nicolas Joly

Biological Software and Databanks.
Institut Pasteur, Paris.

Re: CVS commit: src/lib/libc/stdlib

[Matthias Scheler]

On Thu, Sep 30, 2010 at 03:35:41PM +0200, Nicolas Joly wrote:
 That may be part of the problem (at least for zsh 4.2).
 
 http://www.opengroup.org/onlinepubs/009695399/functions/putenv.html
 
 From the OpenGroup function description; this function does not copy
 the provided string, but use it directly instead. It's the caller
 responsability to clean it when not in use anymore.

I see.

 zsh 4.2 seems to follow this and try to deallocate the previous
 variable string when it has been replaced by a new one. But our
 putenv, which calls setenv, has already done the same ...

So it assumes that it gets back from getenv(3) exactly what it passed
into putenv(3). Well, it seesm our putenv(3) needs a rewrite.
I think that setenv(3) and unsetenv(3) are correct as they are.

Kind regards

-- 
Matthias Scheler  http://zhadum.org.uk/

Re: CVS commit: src/lib/libc/stdlib

[Nicolas Joly]

On Thu, Sep 30, 2010 at 12:41:34PM +, Matthias Scheler wrote:
 Module Name:  src
 Committed By: tron
 Date: Thu Sep 30 12:41:33 UTC 2010
 
 Modified Files:
   src/lib/libc/stdlib: setenv.c unsetenv.c
 
 Log Message:
 Be slightly more careful about freeing memory allocated for environment
 variables: only free memory if the current value points to the same
 memory area as the allocated block. This will prevent crashes if an
 application changes the order of the environment array.
 
 Unfortunately this is still not enough to stop zsh 4.2.* from crashing.
 zsh 4.3.* works fine before and after this change.

Thanks Matthias,

One possibility could be to not free memory at all in setenv, but only
with unsetenv.

IMHO, it's a programmer error to call setenv more than once on the
same variable without a corresponding unsetenv in between (just like
malloc()/free() behaviour). Otherwise, if i understand things
correctly, it's more putenv that shoud work that way.

-- 
Nicolas Joly

Biological Software and Databanks.
Institut Pasteur, Paris.

Re: CVS commit: src/lib/libc/stdlib

[Christos Zoulas]

In article 20100930134213.ga18...@colwyn.zhadum.org.uk,
Matthias Scheler  t...@netbsd.org wrote:
On Thu, Sep 30, 2010 at 03:35:41PM +0200, Nicolas Joly wrote:
 That may be part of the problem (at least for zsh 4.2).
 
 http://www.opengroup.org/onlinepubs/009695399/functions/putenv.html
 
 From the OpenGroup function description; this function does not copy
 the provided string, but use it directly instead. It's the caller
 responsability to clean it when not in use anymore.

I see.

 zsh 4.2 seems to follow this and try to deallocate the previous
 variable string when it has been replaced by a new one. But our
 putenv, which calls setenv, has already done the same ...

So it assumes that it gets back from getenv(3) exactly what it passed
into putenv(3). Well, it seesm our putenv(3) needs a rewrite.
I think that setenv(3) and unsetenv(3) are correct as they are.

Well, if you fix putenv, please move the saveenv realloc environ code
in __allocenv() so it is not in two places...

christos

Re: CVS commit: src/lib/libc/stdlib

[David Laight]

On Thu, Sep 23, 2010 at 12:02:42PM -0400, Christos Zoulas wrote:
 Module Name:  src
 Committed By: christos
 Date: Thu Sep 23 16:02:41 UTC 2010
 
 Modified Files:
   src/lib/libc/stdlib: setenv.c
 
 Log Message:
 PR/43899: Nicolas Joly: setenv(3)/unsetenv(3) memory leak.
 Partial fix: Don't allocate a new string if the length is equal to the
 old length, because presumably the old string was also nul terminated
 so it has the extra byte needed.
 The real fix is to keep an adjunct array of bits, one for each environment
 variable and keep track if the entry was allocated or not so that we can
 free it in unsetenv.

Hmmm I've just read the TOG page for putenv().
A conformant system isn't required to do anything other than save the
user-supplied pointer.
It even states that you can modify the value by changing the memory buffer
after calling putenv().

If the strings are copied in libc...
When libc starts, all the env strings will be adjacent, and at one end
of the stack area (placed by the kernel during exec).
So presumably the addess of the string can be compared against that
range to determine whether the string was malloc'ed or not.

David

-- 
David Laight: da...@l8s.co.uk

Re: CVS commit: src/lib/libc/stdlib

[enami tsugutomo]

In addition to previous, __allocenv() call in unsetenv() isn't
necessary if we check if there exists the bitmap before touching it.
If still want to call it, it must be protected by the lock.

enami.

Re: CVS commit: src/lib/libc/stdlib

[enami tsugutomo]

 Module Name:  src
 Committed By: christos
 Date: Thu Sep 23 16:02:41 UTC 2010
 
 Modified Files:
   src/lib/libc/stdlib: setenv.c
 
 Log Message:
 PR/43899: Nicolas Joly: setenv(3)/unsetenv(3) memory leak.
 Partial fix: Don't allocate a new string if the length is equal to the
 old length, because presumably the old string was also nul terminated
 so it has the extra byte needed.
 The real fix is to keep an adjunct array of bits, one for each environmen=
 t
 variable and keep track if the entry was allocated or not so that we can
 free it in unsetenv.
 
 
 To generate a diff of this commit:
 cvs rdiff -u -r1.32 -r1.33 src/lib/libc/stdlib/setenv.c

Actual change done in setenv.c rev. 1.33 causes following program
memory leak (while rev. 1.32 does not).

#include stdlib.h

main()
{

while (1)
setenv(dummyvar, dummyval, 1);
}

And rev. 1.34 introduces lock leak on error path while it leaves
another fixable memory leak.

I guess following patch is worth to be applied.

enami.

Index: lib/libc/stdlib/setenv.c
===
RCS file: /cvsroot/src/lib/libc/stdlib/setenv.c,v
retrieving revision 1.34
diff -u -r1.34 setenv.c
--- lib/libc/stdlib/setenv.c23 Sep 2010 17:30:49 -  1.34
+++ lib/libc/stdlib/setenv.c24 Sep 2010 03:43:19 -
@@ -81,7 +81,7 @@
c = __findenv(name, offset);
 
if (__allocenv(offset) == -1)
-   return -1;
+   goto bad;
 
if (*value == '=')  /* no `=' in value */
++value;
@@ -90,7 +90,7 @@
if (c != NULL) {
if (!rewrite)
goto good;
-   if (strlen(c)  l_value)/* old larger; copy over */
+   if (strlen(c) = l_value)   /* old is enough; copy over */
goto copy;
} else {/* create new slot */
size = (size_t)(sizeof(char *) * (offset + 2));
@@ -113,6 +113,8 @@
/* name + `=' + value */
if ((c = malloc(size + l_value + 2)) == NULL)
goto bad;
+   if (bit_test(__environ_malloced, offset))
+   free(environ[offset]);
environ[offset] = c;
(void)memcpy(c, name, size);
c += size;