Re: [9fans] Why getenv replaces \0 with spaces in the returned value?

[Giacomo Tesio]

Yes that would be a plausible explanation but actually rc does not use
getenv: it reads /env/ files directly.

I've tried to remove the loop and I can't see any issue.


Giacomo


2017-01-18 21:13 GMT+01:00 Charles Forsyth :
> Yes, it's the lists. Nothing will cope with \0 in a C string, so it's a good
> choice as list of string element separator.
>
> On 18 January 2017 at 19:21, Fran. J Ballesteros  wrote:
>>
>> rc lists?
>>
>> > El 18 ene 2017, a las 17:45, Giacomo Tesio  escribió:
>> >
>> > Hi, last night I noticed this strange post processing in 4th edition's
>> > getenv:
>> > https://github.com/brho/plan9/blob/master/sys/src/libc/9sys/getenv.c#L34-L41
>> >
>> >seek(f, 0, 0);
>> >r = read(f, ans, s);
>> >if(r >= 0) {
>> >ep = ans + s - 1;
>> >for(p = ans; p < ep; p++)
>> >if(*p == '\0')
>> >*p = ' ';
>> >ans[s] = '\0';
>> >}
>> >
>> > Anybody know why this replacement is done?
>> > It does not seem a good fix to read/write or read/truncate races, but
>> > I can't find a better explanation.
>> >
>> >
>> > Giacomo
>> >
>>
>>
>

Re: [9fans] Why getenv replaces \0 with spaces in the returned value?

[Charles Forsyth]

Yes, it's the lists. Nothing will cope with \0 in a C string, so it's a
good choice as list of string element separator.

On 18 January 2017 at 19:21, Fran. J Ballesteros  wrote:

> rc lists?
>
> > El 18 ene 2017, a las 17:45, Giacomo Tesio  escribió:
> >
> > Hi, last night I noticed this strange post processing in 4th edition's
> > getenv: https://github.com/brho/plan9/blob/master/sys/src/libc/9sys/
> getenv.c#L34-L41
> >
> >seek(f, 0, 0);
> >r = read(f, ans, s);
> >if(r >= 0) {
> >ep = ans + s - 1;
> >for(p = ans; p < ep; p++)
> >if(*p == '\0')
> >*p = ' ';
> >ans[s] = '\0';
> >}
> >
> > Anybody know why this replacement is done?
> > It does not seem a good fix to read/write or read/truncate races, but
> > I can't find a better explanation.
> >
> >
> > Giacomo
> >
>
>
>

Re: [9fans] Why getenv replaces \0 with spaces in the returned value?

[Fran. J Ballesteros]

rc lists?

> El 18 ene 2017, a las 17:45, Giacomo Tesio  escribió:
> 
> Hi, last night I noticed this strange post processing in 4th edition's
> getenv: 
> https://github.com/brho/plan9/blob/master/sys/src/libc/9sys/getenv.c#L34-L41
> 
>seek(f, 0, 0);
>r = read(f, ans, s);
>if(r >= 0) {
>ep = ans + s - 1;
>for(p = ans; p < ep; p++)
>if(*p == '\0')
>*p = ' ';
>ans[s] = '\0';
>}
> 
> Anybody know why this replacement is done?
> It does not seem a good fix to read/write or read/truncate races, but
> I can't find a better explanation.
> 
> 
> Giacomo
> 

Re: [9fans] Why getenv replaces \0 with spaces in the returned value?

[Ryan Gonzalez]

Just a wild guess, but I think it could be because getenv just returns a null-
terminated string with no indication of its length. If C code were to do
pretty much anything on the environment variable in question, it would always
be truncated. e.g. with VAR=ABC\0DEF, the C string processing functions would
assume it ended after ABC.

  

As a side note, Linux does nothing and simply returns it as-is. I wonder why
the Posix guys haven't added a getenv_n(var, ) function yet...

  

\--  

Ryan (ライアン)

Yoko Shimomura > ryo (supercell/EGOIST) > Hiroyuki Sawano >> everyone else



  
On Jan 18 2017, at 10:47 am, Giacomo Tesio  wrote:  

> Hi, last night I noticed this strange post processing in 4th edition's  
getenv:
https://github.com/brho/plan9/blob/master/sys/src/libc/9sys/getenv.c#L34-L41

>

> seek(f, 0, 0);  
r = read(f, ans, s);  
if(r >= 0) {  
ep = ans + s - 1;  
for(p = ans; p < ep; p++)  
if(*p == '\0')  
*p = ' ';  
ans[s] = '\0';  
}

>

> Anybody know why this replacement is done?  
It does not seem a good fix to read/write or read/truncate races, but  
I can't find a better explanation.

>

>  
Giacomo

[9fans] Why getenv replaces \0 with spaces in the returned value?

[Giacomo Tesio]

Hi, last night I noticed this strange post processing in 4th edition's
getenv: 
https://github.com/brho/plan9/blob/master/sys/src/libc/9sys/getenv.c#L34-L41

seek(f, 0, 0);
r = read(f, ans, s);
if(r >= 0) {
ep = ans + s - 1;
for(p = ans; p < ep; p++)
if(*p == '\0')
*p = ' ';
ans[s] = '\0';
}

Anybody know why this replacement is done?
It does not seem a good fix to read/write or read/truncate races, but
I can't find a better explanation.


Giacomo

Re: [9fans] memory leaks in libmp

[Giacomo Tesio]

Oh, well, I'm sorry, I should have clarified the context a bit more. Here
it is.

The link I provided here are Jehanne issues, not Plan 9 or 9front bug
reports.
After fixing a few of them I realized that, an year from now, I won't be
able to remember why I did the change. Also, coverity could shut down and I
would have no hint.

So I started coping the coverity scan comments, as a sort of note to a
future self wondering "Why the hell I changed this code?".

The coverity comments in the Jehanne issues are NOT a proof of anything,
they are just quick reminders for Jehanne's developers.

Given that, I thought: why not share these with 9fans and 9front since they
use that code too but cannot access coverity?

Now, for Jehanne these issues were already marked as "worth considering"
(and in this case "worth fixing").
And I'm fine if you consider them false positive or even use a different
fix, since that force me to challenge my assumptions, to review the code
more and even learn from better programmers than me.
Indeed I really like your feedbacks, because you are very deep in the code
base and it usually take you seconds to understand issues that require me
days to figure out.


But these are still **Jehanne's issues**, they are shared in the hope they
helps but with no warranty! ;-)


As you note, I could do it test-driven, providing a failing test and then
fixing the test. You are right, it would be much more useful to Jehanne and
maybe to the Plan 9 ecosystem too.
But, unfortunately, I'm working alone and I do not have the energy to do
this every single time. And I'm one of those few weird people thinking that
you should not care about code coverage if you don't want to pay for a full
one.

Thus I cannot share patches that you can blindly apply, sorry.


As for the issue in question, I think it's a actually a bug. mp(2) states
that
>
> Mptobe and mptole convert an mpint to a byte array.  The
> former creates a big endian representation, the latter a
> little endian one.  If the destination buf is not nil, it
> specifies the buffer of length blen for the result.  If the
> representation is less than blen bytes, the rest of the
> buffer is zero filled.  If buf is nil, then a buffer is
> allocated and a pointer to it is deposited in the location
> pointed to by bufp. Sign is ignored in these conversions,
> i.e., the byte array version is always positive.

To my eye it should only consider bufp if buf is nil. And bufp should be
not nil in that case.

Thus the fix was simply to add an assert to make if fail fast instead of
leaking memory.

The simple reasoning I did during triage was: consider a pointer to a
struct holding both buf and its length:

 mptole(num, s->buf, s->len, nil)

it will cause the leak if the struct was just zeroed.

In this case I prefer the assert fail to the leak, so that I, as a dumb
guy, would notice the issue more rapidly.


Giacomo



2017-01-18 3:31 GMT+01:00 :

> > Still a lot of coverity defects are actually false positives.
> > I try to signal here only the actual bugs but I might be wrong, thus let
> me
> > know if you find these report useless and I will stop to annoy the list.
>
> i cannot predict the future nor tell you how to spend your time. i'm *not*
> claiming that using static code analysis to find bugs is "useless" per se.
>
> but consider the context in which problems would occur, maybe even describe
> how a bug would manifest itself in some code (thats what takes the time or
> wastes our time when you do not do this), and not just blindly present the
> coverty output as a proof.
>
> --
> cinap
>
>