| Message ID | 20180514141218.28438-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | coverity-model: replay data is considered trusted | expand |
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180514141218.28438-1-pbonzini@redhat.com
Subject: [Qemu-devel] [PATCH] coverity-model: replay data is considered trusted
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
t [tag update] patchew/20180512004311.9299-1-richard.henderson@linaro.org -> patchew/20180512004311.9299-1-richard.henderson@linaro.org
* [new tag] patchew/20180514141218.28438-1-pbonzini@redhat.com -> patchew/20180514141218.28438-1-pbonzini@redhat.com
Switched to a new branch 'test'
bb44707cf0 coverity-model: replay data is considered trusted
=== OUTPUT BEGIN ===
Checking PATCH 1/1: coverity-model: replay data is considered trusted...
ERROR: suspect code indent for conditional statements (5, 9)
#25: FILE: scripts/coverity-model.c:111:
+ if (replay_file) {
+ uint8_t c;
total: 1 errors, 0 warnings, 19 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Paolo Bonzini <pbonzini@redhat.com> writes: > Replay data is not considered a possible attack vector; add a model that > does not use getc so that "tainted data" warnings are suppressed. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > scripts/coverity-model.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c > index c702804f41..576f48de33 100644 > --- a/scripts/coverity-model.c > +++ b/scripts/coverity-model.c > @@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table, /* Tainting */ typedef struct {} name2keysym_t; static int get_keysym(const name2keysym_t *table, const char *name) { int result; if (result > 0) { __coverity_tainted_string_sanitize_content__(name); return result; } else { return 0; > } > } > > + Does the new model go under /* Tainting */ ? If yes, I'd like to have just one blank line here. > +/* Replay data is considered trusted. */ If no, I'd like to insert one here. > +uint8_t replay_get_byte(void) > +{ > + uint8_t byte = 0; > + if (replay_file) { > + uint8_t c; > + byte = c; > + } > + return byte; > +} > + > + > /* > * GLib memory allocation functions. > *
On 15/05/2018 14:00, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Replay data is not considered a possible attack vector; add a model that >> does not use getc so that "tainted data" warnings are suppressed. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> scripts/coverity-model.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c >> index c702804f41..576f48de33 100644 >> --- a/scripts/coverity-model.c >> +++ b/scripts/coverity-model.c >> @@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table, > /* Tainting */ > > typedef struct {} name2keysym_t; > static int get_keysym(const name2keysym_t *table, > const char *name) > { > int result; > if (result > 0) { > __coverity_tainted_string_sanitize_content__(name); > return result; > } else { > return 0; >> } >> } >> >> + > > Does the new model go under /* Tainting */ ? Yes, it does. Any chance you can do the change yourself?... Paolo > If yes, I'd like to have just one blank line here. > >> +/* Replay data is considered trusted. */ > > If no, I'd like to insert one here. > >> +uint8_t replay_get_byte(void) >> +{ >> + uint8_t byte = 0; >> + if (replay_file) { >> + uint8_t c; >> + byte = c; >> + } >> + return byte; >> +} >> + >> + >> /* >> * GLib memory allocation functions. >> *
Paolo Bonzini <pbonzini@redhat.com> writes: > On 15/05/2018 14:00, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Replay data is not considered a possible attack vector; add a model that >>> does not use getc so that "tainted data" warnings are suppressed. >>> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> scripts/coverity-model.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c >>> index c702804f41..576f48de33 100644 >>> --- a/scripts/coverity-model.c >>> +++ b/scripts/coverity-model.c >>> @@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table, >> /* Tainting */ >> >> typedef struct {} name2keysym_t; >> static int get_keysym(const name2keysym_t *table, >> const char *name) >> { >> int result; >> if (result > 0) { >> __coverity_tainted_string_sanitize_content__(name); >> return result; >> } else { >> return 0; >>> } >>> } >>> >>> + >> >> Does the new model go under /* Tainting */ ? > > Yes, it does. Any chance you can do the change yourself?... Gladly :) Reviewed-by: Markus Armbruster <armbru@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes: > Replay data is not considered a possible attack vector; add a model that > does not use getc so that "tainted data" warnings are suppressed. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > scripts/coverity-model.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c > index c702804f41..576f48de33 100644 > --- a/scripts/coverity-model.c > +++ b/scripts/coverity-model.c > @@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table, > } > } > > + > +/* Replay data is considered trusted. */ > +uint8_t replay_get_byte(void) > +{ > + uint8_t byte = 0; > + if (replay_file) { > + uint8_t c; > + byte = c; > + } > + return byte; > +} > + > + > /* > * GLib memory allocation functions. > * Coverity 2018.06 chokes on this: $ cov-make-library -of scripts/coverity-model.xmldb scripts/coverity-model.c output file: scripts/coverity-model.xmldb Compiling scripts/coverity-model.c with command /opt/cov-sa-2018.06/bin/cov-emit --dir /tmp/cov-armbru/930a6fb31e5f464fc1a53354b2deb66b/cov-make-library-emit -w --no_error_recovery --emit_header_functions --no_implicit_decl --preinclude /opt/cov-sa-2018.06/library/decls.h --c scripts/coverity-model.c "scripts/coverity-model.c", line 110: error #20: identifier "replay_file" is undefined if (replay_file) { ^ Emit for file '/work/armbru/qemu/scripts/coverity-model.c' complete. [ERROR] 1 error detected in the compilation of "scripts/coverity-model.c". ERROR: cov-emit returned with code 1 Minimal fix: diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c index 48b112393b..f987ce53b8 100644 --- a/scripts/coverity-model.c +++ b/scripts/coverity-model.c @@ -106,6 +106,7 @@ static int get_keysym(const name2keysym_t *table, /* Replay data is considered trusted. */ uint8_t replay_get_byte(void) { + void *replay_file; uint8_t byte = 0; if (replay_file) { uint8_t c; Alternatively, dumb down to: /* Replay data is considered trusted. */ uint8_t replay_get_byte(void) { uint8_t byte; return byte; } Got a preference?
On 26/06/2018 09:27, Markus Armbruster wrote: > > Minimal fix: > > diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c > index 48b112393b..f987ce53b8 100644 > --- a/scripts/coverity-model.c > +++ b/scripts/coverity-model.c > @@ -106,6 +106,7 @@ static int get_keysym(const name2keysym_t *table, > /* Replay data is considered trusted. */ > uint8_t replay_get_byte(void) > { > + void *replay_file; > uint8_t byte = 0; > if (replay_file) { > uint8_t c; > > Alternatively, dumb down to: > > /* Replay data is considered trusted. */ > uint8_t replay_get_byte(void) > { > uint8_t byte; > return byte; > } I wonder why the online service didn't complain. I guess the dumber version is more than enough! Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 26/06/2018 09:27, Markus Armbruster wrote: >> >> Minimal fix: >> >> diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c >> index 48b112393b..f987ce53b8 100644 >> --- a/scripts/coverity-model.c >> +++ b/scripts/coverity-model.c >> @@ -106,6 +106,7 @@ static int get_keysym(const name2keysym_t *table, >> /* Replay data is considered trusted. */ >> uint8_t replay_get_byte(void) >> { >> + void *replay_file; >> uint8_t byte = 0; >> if (replay_file) { >> uint8_t c; >> >> Alternatively, dumb down to: >> >> /* Replay data is considered trusted. */ >> uint8_t replay_get_byte(void) >> { >> uint8_t byte; >> return byte; >> } > > I wonder why the online service didn't complain. Me too. Of course, I should've run cov-make-library myself. Next time... > I guess the dumber > version is more than enough! I'll post it as a proper patch. Thanks!
diff --git a/scripts/coverity-model.c b/scripts/coverity-model.c index c702804f41..576f48de33 100644 --- a/scripts/coverity-model.c +++ b/scripts/coverity-model.c @@ -103,6 +103,19 @@ static int get_keysym(const name2keysym_t *table, } } + +/* Replay data is considered trusted. */ +uint8_t replay_get_byte(void) +{ + uint8_t byte = 0; + if (replay_file) { + uint8_t c; + byte = c; + } + return byte; +} + + /* * GLib memory allocation functions. *
Replay data is not considered a possible attack vector; add a model that does not use getc so that "tainted data" warnings are suppressed. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- scripts/coverity-model.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)