bug-inetutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: happy hacking welcome to tim rühsen


From: Tim Rühsen
Subject: Re: happy hacking welcome to tim rühsen
Date: Mon, 23 Mar 2020 14:07:39 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0

Excuse me for being so busy on other things currently.

The charset has been lost here on the ML (and possibly the exect
formatting as well). BTW, is UTF-8 the charset of ChangeLog or do you
expect pure US-ASCII ?

May I suggest that Alfred or Mats put the ChangeLog file into an own
branch and we all add commits until we are all happy ? Then we merge
those commits into a single one and add it to master.
Currently it is not clear to me who is the "owner" of that file in
regards of being responsible to put in all the suggestions made here.

Regarding mentioning a "CVE tag": Who is going to retrieve a CVE number
? Or has it been done already ?

Regards, Tim

On 21.03.20 01:31, Mats Erik Andersson wrote:
> Torsdag den 3:e mars 2020, klockan 12:30, skrev Alfred M. Szmidt detta:
>> I re-formatted it and slightly edited it.  Mats, could you throw an
>> eye too?
> 
> I deserve scorn for not having observed this pledge earlier, thus causing
> this counter-productive delay. Do feel entitled to loud outcries!
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      uucpd: Fix buffer overflows.
>>
>>      This fixes several missing string termination issues with strncpy.
>>      Also 'remotehost' with size NI_MAXHOST was strcpy'ed into 'line'
>>      which just had 32 bytes on the stack.
>>
>>      * src/uucpd.c (dologout): Increase 'line' array in size.  Use
>>      exisiting macro SCPYN instead of strncpy.
>             ^-- spelling
> 
> SCPYN has been redefined and improved, which must be noted. The previous
> definition was conditioned on PATH_LASTLOG and HAVE_STRUCT_LASTLOG, but
> the new one gets file global scope. Probably safe to ignore.
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      uucpd: Fix heap buffer overflow.
>>
>>      Usernames >= 56 bytes would overflow the char arrays Username and
>>      Logname.  This change mitigates this, but still silently truncates
>>      these arrays.  Silent truncation should be checked throughout the
>>      code possibly within a more comprehensive code review.
>>
>>      * src/uucpd.c: Increase Username and Logname to 72 bytes.
>>      (doit): Use snprintf instead of sprintf.
>>
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      telnet: Use strdup instead malloc+strcpy.
> 
> Grammar: instead of
> 
>>      * telnet/commands.c (tn): Use strdup instead malloc+strcpy.
> 
> Likewise.
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      telnet: Fix -Wsign-compare in suboption.
>>
>>      * telnet/telnet.c (suboption): Explicit cast to int to silence
>>      -Wsign-compare.
>>
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      telnetd: Silence -Wimplicit-fallthrough.
>>
>>      * telnetd/state.c (willoption): Use /* FALLTHROUGH */ to indicate
>>      fallthrough.
> 
> Using C conventions in text is less inspiring. I find it sufficient to say
> something like "Add a comment to make the use of fallthrough case obvious".
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      telnetd: Silence unused warning for getterminaltype.
>>
>>      * telnetd/utility.c (getterminaltype): Silence warning.
> 
> Tautology of a kind. Possibly: "Use LEN in no-op statement".
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      ftp: Silence -Wimplicit-fallthrough warning.
>>
>>      * ftp/domacro.c (domacro): Use /* FALLTHROUGH */ to indicate
>>      fallthrough.
> 
> Same as next to last.
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      ftp: Silence -Wimplicit-fallthrough.
>>
>>      * ftp/cmds.c (domap): Use /* FALLTHROUGH */ to indicate
>>      fallthrough.
> 
> Likewise. This makes for the third commit that definitely should
> have been lumped together, however well deserved they were.
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      ftp: Fix multipliers for M(ega) and G(iga).
>>
>>      This is *not* a fix as the git comment accidentally states.  It is
>>      to silence -Wimplicit-fallthrough and to avoid multiple
>>      multiplications in case of a non-optimized build.  The 'hashbytes'
>>      type needs to be reviewed later as g/G only allows digits 1 and 2
>>      without integer overflow, which is undefined behavior.
>>
>>      * ftp/cmds.c (sethash): Remove cascaded multiplications.
> 
> I accept the wording here, but personally I do not understand the need of
> suppressing multiple multiplications, probably until I gather more background
> information. Anyway, that the author left the comment on `Cascaded' action
> stay untouched indicates a premature implementation of this commit action.
> 
>>
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      ftp: Fix return value of remglob.
>>
>>      * ftp/cmds.c (remglob): Turn around NULL check.
> 
> This is a touchy matter. The comment is correct, but the commit text gives
> the impession of supplying a vital improvement, so I definitely would require
> a more extended explanation here. Perhaps a mention of some viable failure
> that will be avoided by this change. Even without further analysis I can
> believe that the author made a major discovery at this particular point.
> 
>>
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      ftp: Fix strncpy misuse (leading to buffer overflow).
>>
>>      * ftp/ftp.c (hookup): Terminate string after strncpy.
> 
> Good!
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      ftp: Fix 2x misuse of strncpy (read buffer overflow).
>>
>>      * ftp/ftp.c (pswitch): Correctly set the terminating 0.
> 
> Probably `NUL', not naught. I dislike `2x': "Fix two misuses ...",
> or perhaps "Fix misuse of strncpy twice (read ...".
> By the way: Is this committed change really correctly implemented?
> It works correctly only under the assumption that NTIN is no longer
> than `ip->nti', and NTOUT is no longer than `ip->nto'. It is true
> that the old `strlen' must go away, but presumably the byte assignments
> should be done with
> 
>    (ip->nti)[sizeof (ip->nti) - 1]
>    (ip->nto)[sizeof (ip->nto) - 1]
> 
>>
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      talk: Fix uninitialized variable 'nready'.
>>
>>      * talk/ctl_transact.c (ctl_transact): Initialize nready to 0.
> 
> I believe to recall that local variables were to be capitalized
> in our function descriptions: NREADY. Am I mistaken?
> To mention `nready' already in the headline is only confusing at best.
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      whois: Silence -Wimplicit-fallthrough.
>>
>>      * whois/whois.c (main): Use /* FALLTHROUGH */ to indicate
>>      fallthrough.
> 
> Same as earlier on in identical situations. Fourth triviality.
> 
>> 2020-02-29  Tim Rühsen <address@hidden>
>>
>>      ping, ping6: Silence -Wimplicit-fallthrough.
>>
>>      The comment /* FALLTHROUGH */ is well-known by several C/C++
>>      compilers to indicate an explicit fallthrough.  An alternative is
>>      the gcc attribute 'fallthrough' which is less compatible and thus
>>      not chosen here.
>>
>>      * ping/ping.c (parse_opt): Use /* FALLTHROUGH */ to indicate
>>      fallthrough.
>>      * ping/ping6.c (parse_opt): Likewise.
> 
> Now I understand the repetition, but the character sequences `/*'
> and `*/' are still misplaced in natural languages. Do mention comments
> and the relevant keyword; that clearly suffices in a superior manner.
> Is `compatible' really correct? Portable? There is a missing predicate:
> 
>    which is less portable and is thus not chosen here
> 
>> 2020-02-17  Tim Rühsen <address@hidden>
>>
>>      ping6: Fix memleak in ping_set_dest.
>>
>>      * ping/ping6.c (ping_set_dest): Add 'const' to param 'host'.
>>      Rerrange code to avoid memory leak.
> 
> Spelling: rearrange.
> I am not comfortable with this muted explanation. getaddrinfo() is 
> now working on a different string HOST, and RHOST is jumping around
> between different scopes with changing precompiler conditionals.
> I cannot follow this with a simplistic explanation. How did the
> constant declaration of HOST aid in avoiding memory leakage?
> In conclusion: expand the explanation for the sake of our users.
> 
>>      * ping/ping6.h (ping_set_dest): Add 'const' to param 'host'.
>>
>> 2020-02-17  Tim Rühsen <address@hidden>
>>
>>      ping: Fix memleak in ping_set_dest.
>>
>>      * ping/libping.c (ping_set_dest): Add 'const' to param 'host'.
>>      Rerrange code to avoid memory leak.
> 
> Likewise in every detail. In addition, the exchange of P for RHOST ought
> to be clarified. The comment `P is allocated' stands untouched, but P
> is gone, so I am confused. How come libidn needs more change here,
> than it does for ping6 in the next commit?
> 
>>      * ping/ping.h (ping_set_dest): Add 'const' to param 'host'.
>>
>> 2020-02-16  Tim Rühsen <address@hidden>
>>
>>      libls: Remove unused variable kflag.
> 
> Suppress `kflag' here.
> 
>>      * libls/ls.c (ls_main): Remove unused variable 'kflag'.
>>
>> 2020-02-16  Tim Rühsen <address@hidden>
>>
>>      ftpd: Fix multiple definition of 'errcatch' (gcc 10).
>>
>>      * ftpd/extern.h: Remove 'extern' from 'errcatch'.
> 
> Did you mean the opposite?
> Missing explanation:
> 
>   (new) * ftpd/ftpd.c (errcatch): New declaration.
> 
> I have issued `git diff HEAD~19..HEAD~18' repeatedly on order
> to arrive at this interpretation. Then I have checked this
> with 'sed -n 147p ftpd/ftpd.c'.
> 
>> 2020-02-16  Tim Rühsen <address@hidden>
>>
>>      telnetd: Fix multiple definition of 'not42' (gcc 10).
>>
>>      * telnetd/utility.c: Remove 'extern' from 'not42'.
> 
> In fact: Added `extern'.
> Checked with 'sed -n 66p telnetd/utility.c'.
> 
>> 2020-02-16  Tim Rühsen <address@hidden>
>>
>>      * src/rcp.c (tolocal): Remove unused variable 'len'.
>>
>> 2020-02-16  Tim Rühsen <address@hidden>
>>
>>      telnet: Fix silent truncation (off-by-one check)
>>
>>      If the DISPLAY variable had exactly 44 bytes, the SE byte (end sub
>>      negotiation) was silently truncated.
> 
> Conditional sentence: If something had ..., then whatever would be
> 
>>
>>      * telnet/telnet.c (suboption): Use >= instead of >.
> 
> Tricky sentence, as the mathematical symbols do not adapt well
> in single font scripture, unless quoted somehow. The committed
> change is in itself vital, and is commendable. However, while
> the commit message unfortunately neglected to mention its
> relevance to CVE-2019-0053, it becomes all the more imperative
> that our changelog makes the pertinence obvious, even to some
> casual reader just leisurally perusing the text.
> 
> This reading took much longer than anticipated. Time is well past
> midnight, so I do not bother correcting the inverted reading
> order caused by the correctly implemented time scale.
> 
> Best regards,
> 
>   M E A
> 

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]