| Message ID | 545E95B1.5030703@georgi-clan.de |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On November 8, 2014 11:22:54 PM Patrick Georgi <patrick@georgi-clan.de> wrote: > This could silently fail which leads to surprising behaviour. > > Found-by: Coverity Scan > Signed-off-by: Patrick Georgi <patrick@georgi-clan.de> > --- > src/set.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) This clearly isn't a patch against the Linux kernel, so it'd be good to clarify what repository it should be applied to so people don't have to guess. Thierry -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 09.11.2014 um 02:04 schrieb Thierry Reding: > On November 8, 2014 11:22:54 PM Patrick Georgi <patrick@georgi-clan.de> wrote: >> This could silently fail which leads to surprising behaviour. >> >> Found-by: Coverity Scan >> Signed-off-by: Patrick Georgi <patrick@georgi-clan.de> >> --- >> src/set.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) > > This clearly isn't a patch against the Linux kernel, so it'd be good to > clarify what repository it should be applied to so people don't have > to guess. Oops, I'm sorry. This is for https://github.com/NVIDIA/cbootimage Thanks, Patrick
On Sat, Nov 08, 2014 at 11:14:09PM +0100, Patrick Georgi wrote: > This could silently fail which leads to surprising behaviour. > > Found-by: Coverity Scan > Signed-off-by: Patrick Georgi <patrick@georgi-clan.de> > --- > src/set.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/src/set.c b/src/set.c > index ff32b53..907d640 100644 > --- a/src/set.c > +++ b/src/set.c > @@ -59,7 +59,11 @@ read_from_image(char *filename, > return result; > } > > - fseek(fp, offset, SEEK_SET); > + if (fseek(fp, offset, SEEK_SET) == -1) { > + printf("Error: Couldn't seek to %s(%d)\n", filename, offset); offset is unsigned, so the format specifier should be %u. I also wonder whether it would be good to output errno along with the message (or the string representation thereof) to increase the information content. But given that none of the other error messages have that it could be a separate patch. With the %d -> %u for the format specifier, this is: Reviewed-by: Thierry Reding <treding@nvidia.com>
On Mon, Nov 10, 2014 at 09:51:03AM +0100, Thierry Reding wrote: > On Sat, Nov 08, 2014 at 11:14:09PM +0100, Patrick Georgi wrote: > > This could silently fail which leads to surprising behaviour. > > > > Found-by: Coverity Scan > > Signed-off-by: Patrick Georgi <patrick@georgi-clan.de> > > --- > > src/set.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/src/set.c b/src/set.c > > index ff32b53..907d640 100644 > > --- a/src/set.c > > +++ b/src/set.c > > @@ -59,7 +59,11 @@ read_from_image(char *filename, > > return result; > > } > > > > - fseek(fp, offset, SEEK_SET); > > + if (fseek(fp, offset, SEEK_SET) == -1) { > > + printf("Error: Couldn't seek to %s(%d)\n", filename, offset); > > offset is unsigned, so the format specifier should be %u. I also wonder > whether it would be good to output errno along with the message (or the > string representation thereof) to increase the information content. But > given that none of the other error messages have that it could be a > separate patch. > > With the %d -> %u for the format specifier, this is: > > Reviewed-by: Thierry Reding <treding@nvidia.com> I just realized that I have commit access to the cbootimage repository, so I just made the %d -> %u change myself and pushed. Thanks, Thierry
On 11/10/2014 01:51 AM, Thierry Reding wrote: > On Sat, Nov 08, 2014 at 11:14:09PM +0100, Patrick Georgi wrote: >> This could silently fail which leads to surprising behaviour. >> >> Found-by: Coverity Scan >> Signed-off-by: Patrick Georgi <patrick@georgi-clan.de> >> --- >> src/set.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/src/set.c b/src/set.c >> index ff32b53..907d640 100644 >> --- a/src/set.c >> +++ b/src/set.c >> @@ -59,7 +59,11 @@ read_from_image(char *filename, >> return result; >> } >> >> - fseek(fp, offset, SEEK_SET); >> + if (fseek(fp, offset, SEEK_SET) == -1) { >> + printf("Error: Couldn't seek to %s(%d)\n", filename, offset); > > offset is unsigned, so the format specifier should be %u. I also wonder > whether it would be good to output errno along with the message (or the > string representation thereof) to increase the information content. But > given that none of the other error messages have that it could be a > separate patch. Using perror() might be useful? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 10, 2014 at 09:31:55AM -0700, Stephen Warren wrote: > On 11/10/2014 01:51 AM, Thierry Reding wrote: > >On Sat, Nov 08, 2014 at 11:14:09PM +0100, Patrick Georgi wrote: > >>This could silently fail which leads to surprising behaviour. > >> > >>Found-by: Coverity Scan > >>Signed-off-by: Patrick Georgi <patrick@georgi-clan.de> > >>--- > >> src/set.c | 6 +++++- > >> 1 file changed, 5 insertions(+), 1 deletion(-) > >> > >>diff --git a/src/set.c b/src/set.c > >>index ff32b53..907d640 100644 > >>--- a/src/set.c > >>+++ b/src/set.c > >>@@ -59,7 +59,11 @@ read_from_image(char *filename, > >> return result; > >> } > >> > >>- fseek(fp, offset, SEEK_SET); > >>+ if (fseek(fp, offset, SEEK_SET) == -1) { > >>+ printf("Error: Couldn't seek to %s(%d)\n", filename, offset); > > > >offset is unsigned, so the format specifier should be %u. I also wonder > >whether it would be good to output errno along with the message (or the > >string representation thereof) to increase the information content. But > >given that none of the other error messages have that it could be a > >separate patch. > > Using perror() might be useful? Indeed, that would also make these messages go to stderr instead of stdout and be more idiomatic. Thierry
diff --git a/src/set.c b/src/set.c index ff32b53..907d640 100644 --- a/src/set.c +++ b/src/set.c @@ -59,7 +59,11 @@ read_from_image(char *filename, return result; } - fseek(fp, offset, SEEK_SET); + if (fseek(fp, offset, SEEK_SET) == -1) { + printf("Error: Couldn't seek to %s(%d)\n", filename, offset); + result = 1; + goto cleanup; + } if (stat(filename, &stats) != 0) { printf("Error: Unable to query info on bootloader path %s\n",
This could silently fail which leads to surprising behaviour. Found-by: Coverity Scan Signed-off-by: Patrick Georgi <patrick@georgi-clan.de> --- src/set.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)