[vlc-devel] [PATCH 05/15] httpd: Fix potential null-dereference in case of error

[Pierre Ynard]

> From: Hugo Beauzée-Luyssen <hugo at beauzee.fr>
> 
> ---
>  src/network/httpd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/network/httpd.c b/src/network/httpd.c
> index e11c9aab99..b2267e5de2 100644
> --- a/src/network/httpd.c
> +++ b/src/network/httpd.c
> @@ -424,10 +424,12 @@ httpd_HandlerCallBack(httpd_callback_sys_t *p_sys, httpd_client_t *cl,
>          *psz_remote_addr = '\0';
>  
>      uint8_t *psz_args = query->psz_args;
> -    handler->pf_fill(handler->p_sys, handler, query->psz_url, psz_args,
> -                      query->i_type, query->p_body, query->i_body,
> -                      psz_remote_addr, NULL,
> -                      &answer->p_body, &answer->i_body);
> +    if ( handler->pf_fill(handler->p_sys, handler, query->psz_url, psz_args,
> +                          query->i_type, query->p_body, query->i_body,
> +                          psz_remote_addr, NULL,
> +                          &answer->p_body, &answer->i_body) != VLC_SUCCESS)
> +        return VLC_EGENERIC;
> +
>  
>      if (query->i_type == HTTPD_MSG_HEAD) {
>          char *p = (char *)answer->p_body;

This patch looks incorrect to me, in several aspects. First, the
pf_fill() callback could still return success but a buffer that is
missing or too short. Instead, answer->i_body should be checked and all
following strchr(), strncmp() and strtol() might need to be modified.

Then checking for and returning errors might be orthogonal to this.
The semantics of these callbacks are not really defined or documented.
There are two levels here: the high-level httpd_FileNew() and
httpd_HandlerNew() APIs with the callbacks passed to them, and the
low-level httpd_UrlCatch() that they internally use and register with.

httpd_FileNew() and httpd_HandlerNew() have never checked the return
value of their pf_fill() callback. We can't know for sure how any
out-of-tree module might have been using them. But I think we should
check them to return HTTP 500 errors. The file callback currently has no
way of returning an error, so this could be it; and the answer body it
might still return may or may not be used for this 500 error, or an HTML
error page may or may not be generated instead.

Handler callbacks are supposed to write their own HTTP headers, but they
still default to HTTP 200 if no Status is passed back; so there too I
think errors should default to HTTP 500 instead, and the returned body
still treated regardless.

The third matter is returning VLC_EGENERIC. As I just explained, I think
it would be more functional to successfully process high-level callback
errors into HTTP 500 errors, than to return low-level errors. But then I
don't even really understand the semantics of such a low-level error.

Most callbacks registered with httpd_UrlCatch() never fail: in all
in-tree users, the only two times a callback returns an error is when
the RTSP stack receives a request type for which it didn't register
(might nearly as well be an assert), or when the HTTP stream output has
no data to output.

The only place these errors are handled is in the request dispatch loop
trying to match and find the right callback, by:

> if (url->catch[i_msg].cb(url->catch[i_msg].p_sys, cl, answer, query))
>     continue;

telling the httpd, in case of error, to keep looking for another
callback. This confuses me, and the only possible hint I can find is
this comment, originally written in 2004:

> /* all registered url (becarefull that 2 httpd_url_t could point at the same url)
>  * This will slow down the url research but make my live easier
>  * All url will have their cb trigger, but only the first one can answer
>  * */
> struct vlc_list urls;

Is there anybody who understand this code who could explain, or even add
some comments into it?

In any case, I'm skeptical that returning VLC_EGENERIC from
httpd_HandlerCallBack() is the right thing to do, and I'd recommend
against it.

-- 
Pierre Ynard
"Une âme dans un corps, c'est comme un dessin sur une feuille de papier."