fwrite() - possible division by zero

fwrite() - possible division by zero

[Geraldo Netto]

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]

Dear Friends,

I was playing with musl and I think I may have found an issue on fwrite():

This is the original code:

size_t fwrite(const void *restrict src, size_t size, size_t nmemb,
FILE *restrict f)
{
    size_t k, l = size*nmemb;
    if (!size) nmemb = 0;
    FLOCK(f);
    k = __fwritex(src, l, f);
    FUNLOCK(f);
    return k==l ? nmemb : k/size;
}


It seems we need to check the variable size on return because if size is zero
We'll have a division by zero and a segmentation fault

I'm sending the attached patch that changes the return as follows:

return k==l ? nmemb : (size != 0) ? k/size : k;


I don't know if this is the correct approach, so, feel free to
change/let me know how to fix :)
Hope it helps


Kind Regards,

Geraldo Netto
Sapere Aude => Non dvcor, dvco
http://exdev.sf.net/

[-- Attachment #2: 0001-fwrite-avoid-segmentation-fault-when-size-0.patch --]
[-- Type: application/octet-stream, Size: 774 bytes --]

From b1679c6aa916eafe0a490a246b3e6889ce98ac72 Mon Sep 17 00:00:00 2001
From: geraldo netto <geraldonetto@gmail.com>
Date: Wed, 14 Feb 2018 16:19:46 -0200
Subject: [PATCH] fwrite: avoid segmentation fault when size = 0

Signed-off-by: geraldo netto <geraldonetto@gmail.com>
---
 src/stdio/fwrite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/stdio/fwrite.c b/src/stdio/fwrite.c
index 7a567b2..16d2041 100644
--- a/src/stdio/fwrite.c
+++ b/src/stdio/fwrite.c
@@ -32,7 +32,7 @@ size_t fwrite(const void *restrict src, size_t size, size_t nmemb, FILE *restric
 	FLOCK(f);
 	k = __fwritex(src, l, f);
 	FUNLOCK(f);
-	return k==l ? nmemb : k/size;
+	return k==l ? nmemb : (size != 0) ? k/size : k;
 }
 
 weak_alias(fwrite, fwrite_unlocked);
-- 
2.7.4

Re: fwrite() - possible division by zero

[Markus Wichmann]

On Wed, Feb 14, 2018 at 04:24:16PM -0200, Geraldo Netto wrote:
> Dear Friends,
> 
> I was playing with musl and I think I may have found an issue on fwrite():
> 
> This is the original code:
> 
> size_t fwrite(const void *restrict src, size_t size, size_t nmemb,
> FILE *restrict f)
> {
>     size_t k, l = size*nmemb;
>     if (!size) nmemb = 0;
>     FLOCK(f);
>     k = __fwritex(src, l, f);
>     FUNLOCK(f);
>     return k==l ? nmemb : k/size;
> }
> 
> 
> It seems we need to check the variable size on return because if size is zero
> We'll have a division by zero and a segmentation fault
> 

If size is zero, then l is zero. So __fwritex will be called with l as
zero. Which means, if you read that code, that it will have to return
zero. So in the end, k will be zero as well, so k==l, so nmemb will be
returned (which was set to zero earlier), and more importantly, no
division takes place.

> I'm sending the attached patch that changes the return as follows:
> 
> return k==l ? nmemb : (size != 0) ? k/size : k;
> 
> 

Also style: Usual style for musl is to write comparisons with zero as
boolean operations, and to use as few parentheses as possible, i.e.

return k==l ? nmemb : size ? k/size : k;

HTH,
Markus

Re: fwrite() - possible division by zero

[Andrew Bell]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On Wed, Feb 14, 2018 at 2:39 PM, Markus Wichmann <nullplan@gmx.net> wrote:

> On Wed, Feb 14, 2018 at 04:24:16PM -0200, Geraldo Netto wrote:
> > Dear Friends,
> >
> > I was playing with musl and I think I may have found an issue on
> fwrite():
> >
> > This is the original code:
> >
> > size_t fwrite(const void *restrict src, size_t size, size_t nmemb,
> > FILE *restrict f)
> > {
> >     size_t k, l = size*nmemb;
> >     if (!size) nmemb = 0;
> >     FLOCK(f);
> >     k = __fwritex(src, l, f);
> >     FUNLOCK(f);
> >     return k==l ? nmemb : k/size;
> > }
> >
>
> If size is zero, then l is zero. So __fwritex will be called with l as
> zero. Which means, if you read that code, that it will have to return
> zero.


Why not early return if size == 0 and avoid the call to __fwritex
altogether?

-- 
Andrew Bell
andrew.bell.ia@gmail.com

[-- Attachment #2: Type: text/html, Size: 1452 bytes --]

Re: fwrite() - possible division by zero

[Markus Wichmann]

On Wed, Feb 14, 2018 at 02:48:14PM -0500, Andrew Bell wrote:
> Why not early return if size == 0 and avoid the call to __fwritex
> altogether?
> 

Because it's a rare corner case? Here, there's also locking correctness
to consider: fwrite() has to block until f is unlocked, irrespective of
parameters. So there's no real benefit to doing an early return.

Ciao,
Markus

Re: fwrite() - possible division by zero

[Andrew Bell]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Wed, Feb 14, 2018 at 3:07 PM, Markus Wichmann <nullplan@gmx.net> wrote:

> On Wed, Feb 14, 2018 at 02:48:14PM -0500, Andrew Bell wrote:
> > Why not early return if size == 0 and avoid the call to __fwritex
> > altogether?
> >
>
> Because it's a rare corner case? Here, there's also locking correctness
> to consider: fwrite() has to block until f is unlocked, irrespective of
> parameters. So there's no real benefit to doing an early return.
>

But it's already being checked to set nmemb to 0.  Couldn't you just return
0 and avoid the lock as well?

-- 
Andrew Bell
andrew.bell.ia@gmail.com

[-- Attachment #2: Type: text/html, Size: 1130 bytes --]

Re: fwrite() - possible division by zero

[Szabolcs Nagy]

* Andrew Bell <andrew.bell.ia@gmail.com> [2018-02-14 15:11:34 -0500]:
> On Wed, Feb 14, 2018 at 3:07 PM, Markus Wichmann <nullplan@gmx.net> wrote:
> 
> > On Wed, Feb 14, 2018 at 02:48:14PM -0500, Andrew Bell wrote:
> > > Why not early return if size == 0 and avoid the call to __fwritex
> > > altogether?
> > >
> >
> > Because it's a rare corner case? Here, there's also locking correctness
> > to consider: fwrite() has to block until f is unlocked, irrespective of
> > parameters. So there's no real benefit to doing an early return.
> >
> 
> But it's already being checked to set nmemb to 0.  Couldn't you just return
> 0 and avoid the lock as well?

the lock must not be avoided.

otherwise fwrite would make progress on a FILE locked by
another thread which is non-conforming.

the code looks perfectly fine to me.

Re: fwrite() - possible division by zero

[Andrew Bell]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

On Wed, Feb 14, 2018 at 4:15 PM, Szabolcs Nagy <nsz@port70.net> wrote:

> * Andrew Bell <andrew.bell.ia@gmail.com> [2018-02-14 15:11:34 -0500]:
> > On Wed, Feb 14, 2018 at 3:07 PM, Markus Wichmann <nullplan@gmx.net>
> wrote:
> >
> > > On Wed, Feb 14, 2018 at 02:48:14PM -0500, Andrew Bell wrote:
> > > > Why not early return if size == 0 and avoid the call to __fwritex
> > > > altogether?
> > > >
> > >
> > > Because it's a rare corner case? Here, there's also locking correctness
> > > to consider: fwrite() has to block until f is unlocked, irrespective of
> > > parameters. So there's no real benefit to doing an early return.
> > >
> >
> > But it's already being checked to set nmemb to 0.  Couldn't you just
> return
> > 0 and avoid the lock as well?
>
> the lock must not be avoided.
>
> otherwise fwrite would make progress on a FILE locked by
> another thread which is non-conforming.


That's not how I read this: http://port70.net/~nsz/c/c11/n1570.html#7.21.2p8

"All functions that read, write, position, or query the position of a
stream lock the stream before accessing it.
They release the lock associated with the stream when the access is
complete."

When size == 0, the FILE doesn't need to be accessed so no lock should be
necessary.
Perhaps language of this document has been superseded?

But it doesn't much matter.  It just seemed to make the code more clear to
me and would have avoided the initial question.

Best,

-- 
Andrew Bell
andrew.bell.ia@gmail.com

[-- Attachment #2: Type: text/html, Size: 2474 bytes --]

Re: fwrite() - possible division by zero

[Rich Felker]

On Wed, Feb 14, 2018 at 04:47:46PM -0500, Andrew Bell wrote:
> On Wed, Feb 14, 2018 at 4:15 PM, Szabolcs Nagy <nsz@port70.net> wrote:
> 
> > * Andrew Bell <andrew.bell.ia@gmail.com> [2018-02-14 15:11:34 -0500]:
> > > On Wed, Feb 14, 2018 at 3:07 PM, Markus Wichmann <nullplan@gmx.net>
> > wrote:
> > >
> > > > On Wed, Feb 14, 2018 at 02:48:14PM -0500, Andrew Bell wrote:
> > > > > Why not early return if size == 0 and avoid the call to __fwritex
> > > > > altogether?
> > > > >
> > > >
> > > > Because it's a rare corner case? Here, there's also locking correctness
> > > > to consider: fwrite() has to block until f is unlocked, irrespective of
> > > > parameters. So there's no real benefit to doing an early return.
> > > >
> > >
> > > But it's already being checked to set nmemb to 0.  Couldn't you just
> > return
> > > 0 and avoid the lock as well?
> >
> > the lock must not be avoided.
> >
> > otherwise fwrite would make progress on a FILE locked by
> > another thread which is non-conforming.
> 
> 
> That's not how I read this: http://port70.net/~nsz/c/c11/n1570.html#7.21.2p8
> 
> "All functions that read, write, position, or query the position of a
> stream lock the stream before accessing it.
> They release the lock associated with the stream when the access is
> complete."
> 
> When size == 0, the FILE doesn't need to be accessed so no lock should be
> necessary.
> Perhaps language of this document has been superseded?
> 
> But it doesn't much matter.  It just seemed to make the code more clear to
> me and would have avoided the initial question.

It's a POSIX requirement:

	"All functions that reference (FILE *) objects, except those
	with names ending in _unlocked, shall behave as if they use
	flockfile() and funlockfile() internally to obtain ownership
	of these (FILE *) objects."

http://pubs.opengroup.org/onlinepubs/9699919799/functions/flockfile.html

Rich

Re: fwrite() - possible division by zero

[Geraldo Netto]

[-- Attachment #1: Type: text/plain, Size: 2415 bytes --]

Hello Guys,

Thanks for your support/assistance and my apologies for the trouble
I'm trying to learn more about musl to upgrade musl version on osv
But as you could notice, i'm completely newbie in this, oops!

Anyway, let's keep in touch


Kind Regards,

Geraldo Netto
Sapere Aude => Non dvcor, dvco
http://exdev.sf.net/

On 15 February 2018 at 02:20, Rich Felker <dalias@libc.org> wrote:

> On Wed, Feb 14, 2018 at 04:47:46PM -0500, Andrew Bell wrote:
> > On Wed, Feb 14, 2018 at 4:15 PM, Szabolcs Nagy <nsz@port70.net> wrote:
> >
> > > * Andrew Bell <andrew.bell.ia@gmail.com> [2018-02-14 15:11:34 -0500]:
> > > > On Wed, Feb 14, 2018 at 3:07 PM, Markus Wichmann <nullplan@gmx.net>
> > > wrote:
> > > >
> > > > > On Wed, Feb 14, 2018 at 02:48:14PM -0500, Andrew Bell wrote:
> > > > > > Why not early return if size == 0 and avoid the call to __fwritex
> > > > > > altogether?
> > > > > >
> > > > >
> > > > > Because it's a rare corner case? Here, there's also locking
> correctness
> > > > > to consider: fwrite() has to block until f is unlocked,
> irrespective of
> > > > > parameters. So there's no real benefit to doing an early return.
> > > > >
> > > >
> > > > But it's already being checked to set nmemb to 0.  Couldn't you just
> > > return
> > > > 0 and avoid the lock as well?
> > >
> > > the lock must not be avoided.
> > >
> > > otherwise fwrite would make progress on a FILE locked by
> > > another thread which is non-conforming.
> >
> >
> > That's not how I read this: http://port70.net/~nsz/c/c11/
> n1570.html#7.21.2p8
> >
> > "All functions that read, write, position, or query the position of a
> > stream lock the stream before accessing it.
> > They release the lock associated with the stream when the access is
> > complete."
> >
> > When size == 0, the FILE doesn't need to be accessed so no lock should be
> > necessary.
> > Perhaps language of this document has been superseded?
> >
> > But it doesn't much matter.  It just seemed to make the code more clear
> to
> > me and would have avoided the initial question.
>
> It's a POSIX requirement:
>
>         "All functions that reference (FILE *) objects, except those
>         with names ending in _unlocked, shall behave as if they use
>         flockfile() and funlockfile() internally to obtain ownership
>         of these (FILE *) objects."
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/flockfile.html
>
> Rich
>

[-- Attachment #2: Type: text/html, Size: 4086 bytes --]