Re: [PATCH 0/4] vhost-user: avoid g_return_val_if() in get/set_config()
On Wed, Nov 18, 2020 at 07:21:15PM +0400, Marc-André Lureau wrote: > Hi > > On Wed, Nov 18, 2020 at 1:17 PM Stefan Hajnoczi wrote: > > > Markus Armbruster pointed out that g_return_val_if() is meant for > > programming > > errors. It must not be used for input validation since it can be compiled > > out. > > Use explicit if statements instead. > > > > This patch series converts vhost-user device backends that use > > g_return_val_if() in get/set_config(). > > > > Stefan Hajnoczi (4): > > contrib/vhost-user-blk: avoid g_return_val_if() input validation > > contrib/vhost-user-gpu: avoid g_return_val_if() input validation > > contrib/vhost-user-input: avoid g_return_val_if() input validation > > block/export: avoid g_return_val_if() input validation > > > > > The condition is the same for all the patches, checking the message config > payload is large enough. Afaict, the value is set by the client, so it > could be a runtime error, and thus explicit checking is required. > > Nevertheless, one nice thing about g_return* macros, is that it provides an > error message when the condition fails, which helps. Could you replace it? > > (fwiw, I think g_return* macros are so convenient, I would simply make > G_DISABLE_CHECKS forbidden and call it a day) I'll add an error message in v2. Stefan signature.asc Description: PGP signature
Re: [PATCH 0/4] vhost-user: avoid g_return_val_if() in get/set_config()
On Wed, Nov 18, 2020 at 09:16:40AM +, Stefan Hajnoczi wrote: Markus Armbruster pointed out that g_return_val_if() is meant for programming errors. It must not be used for input validation since it can be compiled out. Use explicit if statements instead. This patch series converts vhost-user device backends that use g_return_val_if() in get/set_config(). Stefan Hajnoczi (4): contrib/vhost-user-blk: avoid g_return_val_if() input validation contrib/vhost-user-gpu: avoid g_return_val_if() input validation contrib/vhost-user-input: avoid g_return_val_if() input validation block/export: avoid g_return_val_if() input validation block/export/vhost-user-blk-server.c| 4 +++- contrib/vhost-user-blk/vhost-user-blk.c | 4 +++- contrib/vhost-user-gpu/vhost-user-gpu.c | 4 +++- contrib/vhost-user-input/main.c | 4 +++- 4 files changed, 12 insertions(+), 4 deletions(-) Reviewed-by: Stefano Garzarella
Re: [PATCH 0/4] vhost-user: avoid g_return_val_if() in get/set_config()
Stefan Hajnoczi writes: > Markus Armbruster pointed out that g_return_val_if() is meant for programming > errors. It must not be used for input validation since it can be compiled out. > Use explicit if statements instead. > > This patch series converts vhost-user device backends that use > g_return_val_if() in get/set_config(). Reviewed-by: Markus Armbruster
Re: [PATCH 0/4] vhost-user: avoid g_return_val_if() in get/set_config()
Marc-André Lureau writes: > Hi > > On Wed, Nov 18, 2020 at 1:17 PM Stefan Hajnoczi wrote: > >> Markus Armbruster pointed out that g_return_val_if() is meant for >> programming >> errors. It must not be used for input validation since it can be compiled >> out. >> Use explicit if statements instead. >> >> This patch series converts vhost-user device backends that use >> g_return_val_if() in get/set_config(). >> >> Stefan Hajnoczi (4): >> contrib/vhost-user-blk: avoid g_return_val_if() input validation >> contrib/vhost-user-gpu: avoid g_return_val_if() input validation >> contrib/vhost-user-input: avoid g_return_val_if() input validation >> block/export: avoid g_return_val_if() input validation >> >> > The condition is the same for all the patches, checking the message config > payload is large enough. Afaict, the value is set by the client, so it > could be a runtime error, and thus explicit checking is required. > > Nevertheless, one nice thing about g_return* macros, is that it provides an > error message when the condition fails, which helps. Could you replace it? > > (fwiw, I think g_return* macros are so convenient, I would simply make > G_DISABLE_CHECKS forbidden and call it a day) Nice or not, they are as inappropriate for input validation as assert() is: If expr evaluates to FALSE, the current function should be considered to have undefined behaviour (a programmer error). The only correct solution to such an error is to change the module that is calling the current function, so that it avoids this incorrect call.
Re: [PATCH 0/4] vhost-user: avoid g_return_val_if() in get/set_config()
Hi On Wed, Nov 18, 2020 at 1:17 PM Stefan Hajnoczi wrote: > Markus Armbruster pointed out that g_return_val_if() is meant for > programming > errors. It must not be used for input validation since it can be compiled > out. > Use explicit if statements instead. > > This patch series converts vhost-user device backends that use > g_return_val_if() in get/set_config(). > > Stefan Hajnoczi (4): > contrib/vhost-user-blk: avoid g_return_val_if() input validation > contrib/vhost-user-gpu: avoid g_return_val_if() input validation > contrib/vhost-user-input: avoid g_return_val_if() input validation > block/export: avoid g_return_val_if() input validation > > The condition is the same for all the patches, checking the message config payload is large enough. Afaict, the value is set by the client, so it could be a runtime error, and thus explicit checking is required. Nevertheless, one nice thing about g_return* macros, is that it provides an error message when the condition fails, which helps. Could you replace it? (fwiw, I think g_return* macros are so convenient, I would simply make G_DISABLE_CHECKS forbidden and call it a day) -- Marc-André Lureau
[PATCH 0/4] vhost-user: avoid g_return_val_if() in get/set_config()
Markus Armbruster pointed out that g_return_val_if() is meant for programming errors. It must not be used for input validation since it can be compiled out. Use explicit if statements instead. This patch series converts vhost-user device backends that use g_return_val_if() in get/set_config(). Stefan Hajnoczi (4): contrib/vhost-user-blk: avoid g_return_val_if() input validation contrib/vhost-user-gpu: avoid g_return_val_if() input validation contrib/vhost-user-input: avoid g_return_val_if() input validation block/export: avoid g_return_val_if() input validation block/export/vhost-user-blk-server.c| 4 +++- contrib/vhost-user-blk/vhost-user-blk.c | 4 +++- contrib/vhost-user-gpu/vhost-user-gpu.c | 4 +++- contrib/vhost-user-input/main.c | 4 +++- 4 files changed, 12 insertions(+), 4 deletions(-) -- 2.28.0
