Code review comment for ~sergiodj/ubuntu/+source/openssh:bug1877454-hangs-authorizedkeyscommand

On Thursday, May 14 2020, Rafael David Tinoco wrote:

> All good. You're very careful and detailed with all that you do and
> that is a very good signal. Anyone reviewing your cases can see you
> clearly focus in minor details. I particularly like that style and
> usually read everything written in cases, even if big comments and
> diccussions.
>
> Nevertheless, for the SRU template, that might not be true for the SRU
> reviewers. Not that they may dislike reading complete analysis.. but
> sometimes they lack time. I'll give an example so things are more
> clear:

Thank you very much for the review and the comments, Rafael.

> ----------------------------
>
> """
> [Regression Potential]
>
> Since the affected code deals with executing a subprocess, reading its
> output through a pipe, and then relying on wait(2) to determine
> whether the subprocess exited correctly; and considering that this
> code is written in C without the help of features like RAII and with
> the use of goto statements, we are not able to disconsider the chances
> of making a mistake and forgetting to free a resource or to properly
> read/write from/to pipes, for example. This is, after all, the reason
> the bug happened in the first place.
>
> Having said that, openssh contains extensive tests and the code is
> well organized and relatively easy to follow. Upon close inspection,
> there doesn't seem to be an evident problem with the backported fixes.
>
> As usual when dealing with a somewhat older distribution, there is
> always the possibility of encountering problems because we will be
> recompiling openssh using the most recent versions of its build
> dependencies.
> """
>
> I usually use the bullets (from the template) and list one by one:
>
> ----
>
> [Regression Potential]
>
> * code forks and wait(2)s for clean exit, could lead to fd or mem leaks
>
> * upstream code has tests, ran testcase couldn't find leaks
>
> * ran valgrind before/after my fix, no leaks (to mitigate the potential)
>
> ----
>
> 3 bullets and SRU team knows what to expect. IF reviewer thinks is worth investigating further, he/she might look into bug details/comments.

I know we've discussed this already, and I apologize for being too
verbose again. I will make sure to be more succint next time :-).

> ----------------------------
>
> Anyway, apart from my suggestions, your work is very solid.
>
> +1 from me for this merge.

Thanks!

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14