Re: potential bug in atomicio?
On 17 July 2018 14:34:16 CEST, Matt Johnston wrote: >On Wed, Jul 11, 2018 at 05:26:17PM -0300, Daniel Gutson wrote: >> Hi, >> >>considering this: >> >> >https://github.com/mkj/dropbear/blob/d740dc548924f2faf0934e5f9a4b83d2b5d6902d/atomicio.c#L55 >... >> What if res is negative less than -1, for example -2 ? Shouldn't be a >check >> there that res is > 0 ? > >Hi Daniel, > >atomicio() is always to be called with f == read or f == write which >both >won't return a value <-1. Have you found a platform that doesn't do >that? >I'd probably write it as "ret < 0" myself but it was a >straight copy from OpenSSH's tree. I'd also caution against checking against < 0 "just because". Not applying to this code specifically, but for example the documentation of open() says it on error returns -1 but it doesn't say -2 is an invalid return value. For example WinCE DID in fact return negative file handles. The implementation very much LOOKS like it was carefully written to allow reading more than 2GB on 32 bit systems. Then again, looks can be deceiving, and a liberal use of asserts might have been advisable, but better not to change imported code. Regards, Reimar
Re: potential bug in atomicio?
On Wed, Jul 11, 2018 at 05:26:17PM -0300, Daniel Gutson wrote: > Hi, > >considering this: > > https://github.com/mkj/dropbear/blob/d740dc548924f2faf0934e5f9a4b83d2b5d6902d/atomicio.c#L55 ... > What if res is negative less than -1, for example -2 ? Shouldn't be a check > there that res is > 0 ? Hi Daniel, atomicio() is always to be called with f == read or f == write which both won't return a value <-1. Have you found a platform that doesn't do that? I'd probably write it as "ret < 0" myself but it was a straight copy from OpenSSH's tree. Cheers, Matt
Re: potential bug in atomicio?
I'm trying to count the number of ways this code snippet would fail a
commercial coding standard such as MISRA... ;-)
I believe you're right Daniel: res should be bounds checked something like
0 < res < (BUFFER_SIZE-pos) ?? and adding an assert(pos < BUFFER_SIZE)
somewhere would be nice too...
On Wed, 11 Jul 2018 at 21:28, Daniel Gutson wrote:
> Hi,
>
>considering this:
>
> https://github.com/mkj/dropbear/blob/d740dc548924f2faf0934e5f9a4b83d2b5d6902d/atomicio.c#L55
>
>
>
>
> switch (res) {
> case -1:
> if (errno == EINTR || errno == EAGAIN)
> continue;
> return 0;
> case 0:
> errno = EPIPE;
> return pos;
> default:
> pos += (size_t)res;
> }
>
> What if res is negative less than -1, for example -2 ? Shouldn't be a
> check there that res is > 0 ?
>
> Thanks,
>
> Daniel.
>
>
> --
> Who’s got the sweetest disposition?
> One guess, that’s who?
> Who’d never, ever start an argument?
> Who never shows a bit of temperament?
> Who's never wrong but always right?
> Who'd never dream of starting a fight?
> Who get stuck with all the bad luck?
>
