libpano gcc 8 warnings

[Andreas Metzler]

Hello,

building libpano with gcc 8 (instead of 7) triggers a couple of new
warnings that might be interesting:

-----------------------
parser.c: In function 'ReadImageDescription':
parser.c:1854:38: warning: '%s' directive writing up to 65535 bytes into a region of size 256 [-Wformat-overflow=]
sprintf( sBuf.destName, "%s", buf );
^~ ~~~
In file included from /usr/include/stdio.h:862,
from filter.h:26,
from parser.c:47:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:33:10: note: '__builtin___sprintf_chk' output between 1 and 65536 bytes into a destination of size 256
return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
__bos (__s), __fmt, __va_arg_pack ());
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
parser.c:1850:37: warning: '%s' directive writing up to 65535 bytes into a region of size 256 [-Wformat-overflow=]
sprintf( sBuf.srcName, "%s", buf);
^~ ~~~
In file included from /usr/include/stdio.h:862,
from filter.h:26,
from parser.c:47:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:33:10: note: '__builtin___sprintf_chk' output between 1 and 65536 bytes into a destination of size 256
return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
__bos (__s), __fmt, __va_arg_pack ());
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
parser.c: In function 'ParseScript':
parser.c:453:45: warning: '%s' directive writing up to 65535 bytes into a region of size 512 [-Wformat-overflow=]
sprintf( im->name, "%s", buf );
^~ ~~~
In file included from /usr/include/stdio.h:862,
from filter.h:26,
from parser.c:47:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:33:10: note: '__builtin___sprintf_chk' output between 1 and 65536 bytes into a destination of size 512
return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
__bos (__s), __fmt, __va_arg_pack ());
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[...]
correct.c: In function 'getFrame':
correct.c:749:24: warning: '%d' directive writing between 1 and 10 bytes into a region of size 8 [-Wformat-overflow=]
sprintf( percent, "%d", (int) (xul * 100)/(dx>0?dx:1));
^~
correct.c:749:23: note: directive argument in the range [0, 2147483647]
sprintf( percent, "%d", (int) (xul * 100)/(dx>0?dx:1));
^~~~
In file included from /usr/include/stdio.h:862,
from filter.h:26,
from correct.c:35:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:33:10: note: '__builtin___sprintf_chk' output between 2 and 11 bytes into a destination of size 8
return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
__bos (__s), __fmt, __va_arg_pack ());
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[...]
ColourBrightness.c: In function 'ReadHistograms':
ColourBrightness.c:1281:55: warning: '%s' directive writing up to 511 bytes into a region of size 486 [-Wformat-overflow=]
sprintf(tempString2, "Could not open TIFF file [%s]", tempString);
^~ ~~~~~~~~~~
In file included from /usr/include/stdio.h:862,
from filter.h:26,
from ColourBrightness.c:32:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:33:10: note: '__builtin___sprintf_chk' output between 28 and 539 bytes into a destination of size 512
return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
__bos (__s), __fmt, __va_arg_pack ());
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
------------------------------------------------


Also, the following warnings is thrown a couple of times in the test-suite:
Comparing reference images: 2
reference/tiff_m_cropped0000.tif -> tests/tiff_m_cropped0000.tif
TIFFReadDirectory: Warning, Sum of Photometric type-related color channels and ExtraSamples doesn't match SamplesPerPixel. Defining non-color channels as ExtraSamples..
[...]

cu Andreas
--
`What a good friend you are to him, Dr. Maturin. His other friends are
so grateful to you.'
`I sew his ears on from time to time, sure'

[Bruno Postle]

On 23 September 2018 13:48:10 BST, Andreas Metzler wrote:
>
>building libpano with gcc 8 (instead of 7) triggers a couple of new
>warnings that might be interesting:
>

>parser.c: In function 'ReadImageDescription':
>parser.c:1854:38: warning: '%s' directive writing up to 65535 bytes
>into a region of size 256 [-Wformat-overflow=]
> sprintf( sBuf.destName, "%s", buf );
> ^~ ~~~

It looks harmless to me, but my C isn't good enough to say for sure.

Along with the earlier typos I'm inclined to release the release candidate, since it has been waiting so long, then fixes for these can go into the next release.

>Also, the following warnings is thrown a couple of times in the
>test-suite:
>Comparing reference images: 2
>reference/tiff_m_cropped0000.tif -> tests/tiff_m_cropped0000.tif
>TIFFReadDirectory: Warning, Sum of Photometric type-related color
>channels and ExtraSamples doesn't match SamplesPerPixel. Defining
>non-color channels as ExtraSamples..

Seems like libtiff didn't like something about the files rather than the tests themselves failing.

--
Bruno

[Greg 'groggy' Lehey]

On Sunday, 23 September 2018 at 23:43:38 +0100, Bruno Postle wrote:
>
>
> On 23 September 2018 13:48:10 BST, Andreas Metzler wrote:
>>
>> building libpano with gcc 8 (instead of 7) triggers a couple of new
>> warnings that might be interesting:
>>
>> parser.c: In function 'ReadImageDescription':
>> parser.c:1854:38: warning: '%s' directive writing up to 65535 bytes
>> into a region of size 256 [-Wformat-overflow=]
>> sprintf( sBuf.destName, "%s", buf );
>> ^~ ~~~
>
> It looks harmless to me, but my C isn't good enough to say for sure.

I think what it's trying to say is "consider using snprintf ()".

> Along with the earlier typos I'm inclined to release the release
> candidate, since it has been waiting so long, then fixes for these
> can go into the next release.

Sounds reasonable to me.

Greg
--
Sent from my desktop computer.
Finger groo...@gmail.com for PGP public key.
See complete headers for address and phone numbers.
This message is digitally signed. If your Microsoft mail program
reports problems, please read http://lemis.com/broken-MUA

[David W. Jones]

On September 23, 2018 12:43:38 PM HST, Bruno Postle wrote:
>
>
>On 23 September 2018 13:48:10 BST, Andreas Metzler wrote:
>>
>>building libpano with gcc 8 (instead of 7) triggers a couple of new
>>warnings that might be interesting:
>>
>>parser.c: In function 'ReadImageDescription':
>>parser.c:1854:38: warning: '%s' directive writing up to 65535 bytes
>>into a region of size 256 [-Wformat-overflow=]
>> sprintf( sBuf.destName, "%s", buf );
>> ^~ ~~~
>
>It looks harmless to me, but my C isn't good enough to say for sure.

Don't know, either, but I put in an effort compiling a completely different app that reported those kinds of warnings (trying to fit X bytes into something <X). Program would compile but immediately crash on run. Left no debug or log or anything.

>Along with the earlier typos I'm inclined to release the release
>candidate, since it has been waiting so long, then fixes for these can
>go into the next release.
>
>>Also, the following warnings is thrown a couple of times in the
>>test-suite:
>>Comparing reference images: 2
>>reference/tiff_m_cropped0000.tif -> tests/tiff_m_cropped0000.tif
>>TIFFReadDirectory: Warning, Sum of Photometric type-related color
>>channels and ExtraSamples doesn't match SamplesPerPixel. Defining
>>non-color channels as ExtraSamples..
>
>Seems like libtiff didn't like something about the files rather than
>the tests themselves failing.

Sounds like an issue with files themselves, too. Have gotten used to such warnings with images from my camera. It simply doesn't record some of the things libtiff seems to look for.


David W. Jones
gnome...@gmail.com
wandering the landscape of god
http://dancingtreefrog.com

Sent from my Android device with F/LOSS K-9 Mail.

[Gunter Königsmann]

My advise is to replace the sprintf by an snprintf before the final release: snprintf requires an additional parameter that tells it how many bytes the buffer it is about to write into is long; using an ordinary sprintf always means you are risking needing to issue an security update because someone manages to craft a file that tricks snprintf into overwriting the stack or a pointer leading to arbitrary code execution by using return-oriented programming or similar.

And always remember: if snprintf knows that the buffer it writes into is only 255 bytes long and the string to write is 255 bytes long plus the null byte marking the end of the string snprintf won't add a null byte to its end => fill the last byte it the target string with a zero and then tell snprintf the target is only 254 bytes long. Or make the target string longer.

Kind regards, Gunter.

--
A list of frequently asked questions is available at: http://wiki.panotools.org/Hugin_FAQ
---
You received this message because you are subscribed to the Google Groups "hugin and other free panoramic software" group.
To unsubscribe from this group and stop receiving emails from it, send an email to hugin-ptx+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/hugin-ptx/FB46C7F8-A83C-4EED-B71B-A0D9B0EE90D7%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Bruno Postle]

Can we have a patch? This is exactly the sort of thing that I mess up - Bruno

[Rogier Wolff]

On Sun, Sep 23, 2018 at 11:43:38PM +0100, Bruno Postle wrote:
>
>
> On 23 September 2018 13:48:10 BST, Andreas Metzler wrote:
> >
> >building libpano with gcc 8 (instead of 7) triggers a couple of new
> >warnings that might be interesting:
> >
> >parser.c: In function 'ReadImageDescription':
> >parser.c:1854:38: warning: '%s' directive writing up to 65535 bytes
> >into a region of size 256 [-Wformat-overflow=]
> > sprintf( sBuf.destName, "%s", buf );
> > ^~ ~~~

> It looks harmless to me, but my C isn't good enough to say for sure.

The compiler is saying that "sBuf.destName" is declared having a size
of 256, while "buf" is declared as being of size 65536.

When a compiler says such a thing it is usually right.

When this was written, someone probably thought about it and reused
the 65536 byte buffer "buf" for this small task. "buf" Needs to be
65536 bytes long for something else, and is now reused for this
purpose with "max 255" or even less still...

That said... Ignoring these warnings has for years caused serious
security leaks. These warnings didn't exist back then, but we should
take them serious.

In this case,

strncpy ( sBuf.destName, buf, 255);

is a quick rewrite of that specific line that a) avoids the warning
and b) avoids being unsafe even when someone external manages to get
"buf" filled further than expected.
The downside is that the API of strncpy is not convenient and requires
a sBuf.destName[255] = 0; at the end for the code fragment to become
really safe.

I would've liked something along the lines of:

char *mystrncpy (char *dst, char *src, int n)
{
if(!n) return;
n--;
while (*src && n--)
*dst++ = *src++;
*dst++ = 0;
return dst;
}

that always null-terminates the destination string even when the
buffer limit is reached. Alas they did not listen to me when I didn't
know this yet and was only 5 years old.


Roger.



--
+-- Rogier Wolff -- www.harddisk-recovery.nl -- 0800 220 20 20 --
- Datarecovery Services Nederland B.V. Delft. KVK: 30160549 -
| Files foetsie, bestanden kwijt, alle data weg?!
| Blijf kalm en neem contact op met Harddisk-recovery.nl!

[Gunter Königsmann]

Didn't have the time to patch the whole thing. But a first few lines of
the patch would read as attached to this mail - and obviousy would need
an review as everybody gets this kind of thing wrong.

I'm not sure if all the strings we sprintf'ed into actually had the
right length to accomodate the '\0' that ends the string, though...

Kind regards,

     Gunter.