| Message ID | 20240525111814.2587729-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avformat/file: check for lseek() failure | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Le lauantaina 25. toukokuuta 2024, 14.18.12 EEST Michael Niedermayer a écrit :
> Fixes: CID1559855 Unchecked return value from library
This is again very misleading. Logging an error (that most people won't even
see) is not fixing a bug. Either this is not a bug, and this patch can't be
fixing a non-bug, or this is a bug, and this patch is hiding it behind a fake
fix.
In this case, the code looks completely bogus. Restoring the file pointer
before closing the descriptor makes no sense. In the first place, it can only
have an observable effect if the file descriptor is borrowed (i.e. "fd"
protocol). And even then, whatever external code relies on this peculiar
behaviour is irremediably broken, as Coverity highlights. If this is really
needed, the only way that this can work properly is for that external code to
restore the file offset after libavformat is done, and check the error *there*.
Also, AFAIK, by the principle of least surprise, and following the behaviour
of most other programs in similar circumstances, the expected behaviour is
*not* to restore the file pointer.
So IMO, 5c3d2177e7be7dde69a83b1b394f57976c164c84 should be reverted instead.
On Sat, May 25, 2024 at 02:38:07PM +0300, Rémi Denis-Courmont wrote: > Le lauantaina 25. toukokuuta 2024, 14.18.12 EEST Michael Niedermayer a écrit : > > Fixes: CID1559855 Unchecked return value from library > > This is again very misleading. Logging an error (that most people won't even > see) is not fixing a bug. Either this is not a bug, and this patch can't be > fixing a non-bug, or this is a bug, and this patch is hiding it behind a fake > fix. As written in the commit message, the patch would fix CID1559855. It doesnt claim anything else [...] > > So IMO, 5c3d2177e7be7dde69a83b1b394f57976c164c84 should be reverted instead. if thats preferred, i have nothing against doing that instead thx [...]
Le lauantaina 25. toukokuuta 2024, 15.19.51 EEST Michael Niedermayer a écrit : > On Sat, May 25, 2024 at 02:38:07PM +0300, Rémi Denis-Courmont wrote: > > Le lauantaina 25. toukokuuta 2024, 14.18.12 EEST Michael Niedermayer a écrit : > > > Fixes: CID1559855 Unchecked return value from library > > > > This is again very misleading. Logging an error (that most people won't > > even see) is not fixing a bug. Either this is not a bug, and this patch > > can't be fixing a non-bug, or this is a bug, and this patch is hiding it > > behind a fake fix. > > As written in the commit message, the patch would fix CID1559855. > It doesnt claim anything else That does not match my understanding of the verb "to fix". And this is not just a rant about semantics. People may well be mislead and assume that any commit that "fixes" something fixes an FFmpeg bug. > > So IMO, 5c3d2177e7be7dde69a83b1b394f57976c164c84 should be reverted > > instead. > if thats preferred, i have nothing against doing that instead > > thx > > [...]
diff --git a/libavformat/file.c b/libavformat/file.c index 0ed4cff266c..e2ef85aa705 100644 --- a/libavformat/file.c +++ b/libavformat/file.c @@ -222,8 +222,10 @@ static int file_close(URLContext *h) FileContext *c = h->priv_data; int ret; - if (c->initial_pos >= 0 && !h->is_streamed) - lseek(c->fd, c->initial_pos, SEEK_SET); + if (c->initial_pos >= 0 && !h->is_streamed) { + if (lseek(c->fd, c->initial_pos, SEEK_SET) < 0) + av_log(h, AV_LOG_DEBUG, "Failed to restore position (%s)\n", av_err2str(AVERROR(errno))); + } ret = close(c->fd); return (ret == -1) ? AVERROR(errno) : 0;
Fixes: CID1559855 Unchecked return value from library Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/file.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)