[PATCH] Fix 'no format arguments found' warning

[Kannan Rengarajan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Log:
Resolve "format not a string literal and no format arguments found" warning.

* subversion/libsvn_subr/io.c
 (do_io_file_wrapper_cleanup): Add the format specifier "%s", which
  fixes the warning.

Patch by: Kannan R <kannanr@...>

- --
Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEVAwUBSuV9ZHlTqcY7ytmIAQJGfggAqenIlX8SgRVJ9XTzfsauvQLfcZ+wtfuN
iKs/J23Nbjisttx8Pc2QTvQMM7lW1vq3dhi3WuZCRi9hmphYR0pOoSAtPJNURvoW
GGleOvnvDYPZo81GQMDfJRnvNdGnAtixqGpdrh5veF33FoO7CPp3QT/jk1iMWOfl
G0PnzFDGdMz3ASAUlcXb4rL7q3+8HQuC3pIsVAAeoezbS8Jcu0WekQKsvcSDNm7F
eo8TWwYwePpZojso1iYyl8zouimSKu6UDvTZdI9l//6SYyLrgj3kPmhNDFJOFVFZ
VmQDUnQClTj6260vlqDxjCdIUGzZDfTpMp9dvmRB0MVZOYxtzZIEbA==
=kcRC
-----END PGP SIGNATURE-----

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411334
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 40199)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -2790,7 +2790,7 @@
     return svn_error_wrap_apr(status, _(msg),
                               svn_dirent_local_style(name, pool));
   else
-    return svn_error_wrap_apr(status, _(msg_no_name));
+    return svn_error_wrap_apr(status,"%s", _(msg_no_name));
 }
 
 

[Branko Čibej-2]

Kannan wrote:

>
> Log:
> Resolve "format not a string literal and no format arguments found"
> warning.
>
> * subversion/libsvn_subr/io.c
>  (do_io_file_wrapper_cleanup): Add the format specifier "%s", which
>   fixes the warning.
>
> Patch by: Kannan R <kannanr@...>
>

+1 to commit

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411337

[Stefan Sperling-7]

On Mon, Oct 26, 2009 at 04:13:48PM +0530, Kannan wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> Log:
> Resolve "format not a string literal and no format arguments found" warning.
>
> * subversion/libsvn_subr/io.c
>  (do_io_file_wrapper_cleanup): Add the format specifier "%s", which
>   fixes the warning.

We have those warnings all over the place, so if you want to fix
them all you're gonna be busy for a while.
But I'd love to see them fixed, because each of them is a possible
format-string vulnerability.

> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 40199)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -2790,7 +2790,7 @@
>      return svn_error_wrap_apr(status, _(msg),
>                                svn_dirent_local_style(name, pool));

For example, the above call in your patch's context would need
a similar fix.

>    else
> -    return svn_error_wrap_apr(status, _(msg_no_name));
> +    return svn_error_wrap_apr(status,"%s", _(msg_no_name));

We usually put a space after a comma.

Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411344

[Stefan Sperling-7]

On Mon, Oct 26, 2009 at 12:06:41PM +0100, Stefan Sperling wrote:

> On Mon, Oct 26, 2009 at 04:13:48PM +0530, Kannan wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> >
> > Log:
> > Resolve "format not a string literal and no format arguments found" warning.
> >
> > * subversion/libsvn_subr/io.c
> >  (do_io_file_wrapper_cleanup): Add the format specifier "%s", which
> >   fixes the warning.
>
> We have those warnings all over the place, so if you want to fix
> them all you're gonna be busy for a while.
> But I'd love to see them fixed, because each of them is a possible
> format-string vulnerability.

By the way, the proper way to fix this would be to make a list of all
functions used by Subversion which accept a format string, and then go
through this list and check every occurance of each function throughout
the entire code base (grep is your friend).
Once that is done, we need to review all commits as they come in for
changes re-introducing the anti-pattern of passing a buffer where a format
string is expected.

Just relying on the compiler to warn about this could be a bad idea.

Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411347

[Kannan Rengarajan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Stefan Sperling wrote:

> On Mon, Oct 26, 2009 at 12:06:41PM +0100, Stefan Sperling wrote:
>> On Mon, Oct 26, 2009 at 04:13:48PM +0530, Kannan wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>>
>>> Log:
>>> Resolve "format not a string literal and no format arguments found" warning.
>>>
>>> * subversion/libsvn_subr/io.c
>>>  (do_io_file_wrapper_cleanup): Add the format specifier "%s", which
>>>   fixes the warning.
>> We have those warnings all over the place, so if you want to fix
>> them all you're gonna be busy for a while.
>> But I'd love to see them fixed, because each of them is a possible
>> format-string vulnerability.

  The ones which appear in bulk are "format not a string literal,
  argument types not checked" whereas the one(the only one found AFAIK)
  fixed here is that of "format not a string literal and no format
  arguments found". Anyways need to fix them too.

> By the way, the proper way to fix this would be to make a list of all
> functions used by Subversion which accept a format string, and then go
> through this list and check every occurance of each function throughout
> the entire code base (grep is your friend).
> Once that is done, we need to review all commits as they come in for
> changes re-introducing the anti-pattern of passing a buffer where a format
> string is expected.
>
> Just relying on the compiler to warn about this could be a bad idea.

 +1. Herewith I've attached the updated patch as per your comments.


- --
Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEVAwUBSuWIFnlTqcY7ytmIAQKiJwf+J+Nn/0REHSNvecj2pcb8wYPhKiiE7dER
ZQL90XlNPkE0TPt7jw9OwE52eB658//nWCSNrCKDnQJb8fOrdu+8nnlOuX4dRCAe
edJeRJIJlSxJGr8e8GK93yCNZ3dcXhkvQohiCk4hrIdRtnI+8hNFNwHKeFIly/Hn
cm6zt10wqyjGPxn0A7ikSzkanmux8O80c2ZUlcRFio2ir8DqCB47wNPmxWtxavsQ
iCk868/+57eRof7XZIelgc6u+1hYMpQmYnHdKoDD38Wqvaf4TOzGi827NwipqCBt
zd9fVsa+Hgdf//FEoVZljI8rv4i/HrXWkezRYspzKrNaP2679NxVBw==
=K/ZF
-----END PGP SIGNATURE-----

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411350
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 40199)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -2790,7 +2790,7 @@
     return svn_error_wrap_apr(status, _(msg),
                               svn_dirent_local_style(name, pool));
   else
-    return svn_error_wrap_apr(status, _(msg_no_name));
+    return svn_error_wrap_apr(status, "%s", _(msg_no_name));
 }
 
 

[Branko Čibej-2]

Stefan Sperling wrote:

> On Mon, Oct 26, 2009 at 12:06:41PM +0100, Stefan Sperling wrote:
>  
>> On Mon, Oct 26, 2009 at 04:13:48PM +0530, Kannan wrote:
>>    
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>>
>>> Log:
>>> Resolve "format not a string literal and no format arguments found" warning.
>>>
>>> * subversion/libsvn_subr/io.c
>>>  (do_io_file_wrapper_cleanup): Add the format specifier "%s", which
>>>   fixes the warning.
>>>      
>> We have those warnings all over the place, so if you want to fix
>> them all you're gonna be busy for a while.
>> But I'd love to see them fixed, because each of them is a possible
>> format-string vulnerability.
>>    
>
> By the way, the proper way to fix this would be to make a list of all
> functions used by Subversion which accept a format string, and then go
> through this list and check every occurance of each function throughout
> the entire code base (grep is your friend).
> Once that is done, we need to review all commits as they come in for
> changes re-introducing the anti-pattern of passing a buffer where a format
> string is expected.
>
> Just relying on the compiler to warn about this could be a bad idea.
>  

I think we should add properly defined GCC attributes to such functions
declarations, so that we *can* rely on the compiler warning us in
future. APR certainly does that, and even properly defines __attribute__
as a macro when it's not being compiled by GCC.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411358

[Gavin Baumanis-4]

Ping. This thread has not received any more comments.

Gavin.


On 26/10/2009, at 23:09 , Branko Cibej wrote:

> Stefan Sperling wrote:
>> On Mon, Oct 26, 2009 at 12:06:41PM +0100, Stefan Sperling wrote:
>>
>>> On Mon, Oct 26, 2009 at 04:13:48PM +0530, Kannan wrote:
>>>
>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>> Hash: SHA1
>>>>
>>>>
>>>> Log:
>>>> Resolve "format not a string literal and no format arguments  
>>>> found" warning.
>>>>
>>>> * subversion/libsvn_subr/io.c
>>>> (do_io_file_wrapper_cleanup): Add the format specifier "%s", which
>>>>  fixes the warning.
>>>>
>>> We have those warnings all over the place, so if you want to fix
>>> them all you're gonna be busy for a while.
>>> But I'd love to see them fixed, because each of them is a possible
>>> format-string vulnerability.
>>>
>>
>> By the way, the proper way to fix this would be to make a list of all
>> functions used by Subversion which accept a format string, and then  
>> go
>> through this list and check every occurance of each function  
>> throughout
>> the entire code base (grep is your friend).
>> Once that is done, we need to review all commits as they come in for
>> changes re-introducing the anti-pattern of passing a buffer where a  
>> format
>> string is expected.
>>
>> Just relying on the compiler to warn about this could be a bad idea.
>>
>
> I think we should add properly defined GCC attributes to such  
> functions
> declarations, so that we *can* rely on the compiler warning us in
> future. APR certainly does that, and even properly defines  
> __attribute__
> as a macro when it's not being compiled by GCC.
>
> -- Brane
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411358

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413972

[Branko Čibej-2]

Gavin 'Beau' Baumanis wrote:
> Ping. This thread has not received any more comments.

Well, the original patch, aside from a tiny formatting nit, is OK. And I
added the printf-format attribute wherever it was still missing in the
code -- only one place I could find, in fact. All the other similar
warnings I found are harder to resolve, because they legitimately
non-const message formats.


-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414013

[Gavin Baumanis-4]

Hi Brane,

Even with the fear of ending up looking like a bit of an idiot...

Does your reply mean there is nothing left to do?
Have you committed the changes for this patch proposal or do I still  
need to keep it in my "watch-list"?

Gavin.


On 03/11/2009, at 16:00 , Branko Cibej wrote:

> Gavin 'Beau' Baumanis wrote:
>> Ping. This thread has not received any more comments.
>
> Well, the original patch, aside from a tiny formatting nit, is OK.  
> And I
> added the printf-format attribute wherever it was still missing in the
> code -- only one place I could find, in fact. All the other similar
> warnings I found are harder to resolve, because they legitimately
> non-const message formats.
>
>
> -- Brane
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414013

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414084

[Branko Čibej-2]

Gavin 'Beau' Baumanis wrote:
> Hi Brane,
>
> Even with the fear of ending up looking like a bit of an idiot...
>
> Does your reply mean there is nothing left to do?
> Have you committed the changes for this patch proposal or do I still
> need to keep it in my "watch-list"?


Oh, in fact, I assumed Kannan had commit-after-review privileges, and
could commit that patch. I didn't touch the patch, so yes, there's still
stuff to do here. :)

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414090

[Kannan Rengarajan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Branko Čibej wrote:

> Gavin 'Beau' Baumanis wrote:
>> Hi Brane,
>>
>> Even with the fear of ending up looking like a bit of an idiot...
>>
>> Does your reply mean there is nothing left to do?
>> Have you committed the changes for this patch proposal or do I still
>> need to keep it in my "watch-list"?
>
>
> Oh, in fact, I assumed Kannan had commit-after-review privileges, and
> could commit that patch. I didn't touch the patch, so yes, there's still
> stuff to do here. :)

The patch is committed in r40223 by Stefan. Thanks.

- --
Regards,
Kannan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEVAwUBSvBYGXlTqcY7ytmIAQJtAAf/dEHo8f7j/w3yRMKrU1xq+lqlM0MsO4HE
Icu50RJ26JYIE/Aje8G0sc2hFxIIq+sPzNawdKxGW9C4ZooFRvUvUokJ8j2go2wF
NgnL4TiGxj+gSj0q3ROyi28L9fOykA3U5CZCLA7mYFCGewMB6VT+J2kBvq+fPW3c
MAdNvfFEiSPz9dmoZiTOHnCnCgyZvYjrzxKzFMu06zcCMTUmgXlE2kSM4Yy4TKp2
DR55YMDqJR2p6MoZ6F0eTyoeUmtw6ibYFznCLe+2/NMxXgMhO+N1CBodhjaq9hp/
ZHWlbcUIsabOe9rpamNNeh5tHcWbR/TGS1Wmd5Gbh5GRAaYF7lkqWg==
=bNwF
-----END PGP SIGNATURE-----

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414145

[Gavin Baumanis-4]

Thanks for the update - appreciate it.

Gavin.


On 04/11/2009, at 03:19 , Kannan wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Branko Čibej wrote:
>> Gavin 'Beau' Baumanis wrote:
>>> Hi Brane,
>>>
>>> Even with the fear of ending up looking like a bit of an idiot...
>>>
>>> Does your reply mean there is nothing left to do?
>>> Have you committed the changes for this patch proposal or do I still
>>> need to keep it in my "watch-list"?
>>
>>
>> Oh, in fact, I assumed Kannan had commit-after-review privileges, and
>> could commit that patch. I didn't touch the patch, so yes, there's  
>> still
>> stuff to do here. :)
>
> The patch is committed in r40223 by Stefan. Thanks.
>
> - --
> Regards,
> Kannan
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQEVAwUBSvBYGXlTqcY7ytmIAQJtAAf/dEHo8f7j/w3yRMKrU1xq+lqlM0MsO4HE
> Icu50RJ26JYIE/Aje8G0sc2hFxIIq+sPzNawdKxGW9C4ZooFRvUvUokJ8j2go2wF
> NgnL4TiGxj+gSj0q3ROyi28L9fOykA3U5CZCLA7mYFCGewMB6VT+J2kBvq+fPW3c
> MAdNvfFEiSPz9dmoZiTOHnCnCgyZvYjrzxKzFMu06zcCMTUmgXlE2kSM4Yy4TKp2
> DR55YMDqJR2p6MoZ6F0eTyoeUmtw6ibYFznCLe+2/NMxXgMhO+N1CBodhjaq9hp/
> ZHWlbcUIsabOe9rpamNNeh5tHcWbR/TGS1Wmd5Gbh5GRAaYF7lkqWg==
> =bNwF
> -----END PGP SIGNATURE-----
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414217