OpenLDAP
Up to top level
Build   Contrib   Development   Documentation   Historical   Incoming   Software Bugs   Software Enhancements   Web  

Logged in as guest

Viewing Development/4467
Full headers

From: h.b.furuseth@usit.uio.no
Subject: snprintf is consistenly used wrongly
Compose comment
Download message
State:
0 replies:
3 followups: 1 2 3

Major security issue: yes  no

Notes:

Notification:


Date: Sun, 2 Apr 2006 15:01:49 GMT
From: h.b.furuseth@usit.uio.no
To: openldap-its@OpenLDAP.org
Subject: snprintf is consistenly used wrongly
Full_Name: Hallvard B Furuseth
Version: HEAD
OS: 
URL: 
Submission from: (NULL) (129.240.186.42)
Submitted by: hallvard


OpenLDAP is full of code like
  ptr += snprintf( ptr, ... );
and
  bv.bv_len = snprintf( ... );
  <use bv>;

However snprintf does not return the number of characters written, it
returns the number of characters it would write if no truncation occurs,
not including the terminating '\0'.

Correct use is:
  len = snprintf( buf, buflen, ... );
  if ( len >= buflen ) {
    Truncated to (buflen-1) chars + '\0' - or no change if buflen == 0;
  }

I imagine we could use some helper functions here, e.g. a snprintf_long()
for output of just a single arg, but I won't dive into that now.
The snprintf calls that just output a %s can probably just as well
be replaced with something else.


Followup 1

Download message
Subject: Re: (ITS#4467) snprintf is consistenly used wrongly
From: Pierangelo Masarati <ando@sys-net.it>
To: h.b.furuseth@usit.uio.no
Cc: openldap-its@OpenLDAP.org
Date: Sun, 02 Apr 2006 17:26:10 +0200
On Sun, 2006-04-02 at 15:01 +0000, h.b.furuseth@usit.uio.no wrote:
> Full_Name: Hallvard B Furuseth
> Version: HEAD
> OS: 
> URL: 
> Submission from: (NULL) (129.240.186.42)
> Submitted by: hallvard
> 
> 
> OpenLDAP is full of code like
>   ptr += snprintf( ptr, ... );
> and
>   bv.bv_len = snprintf( ... );
>   <use bv>;
> 
> However snprintf does not return the number of characters written, it
> returns the number of characters it would write if no truncation occurs,
> not including the terminating '\0'.

In many cases, especially when used to compute the length of a berval,
snprintf is used under the assumption the buffer is large enough to
contain the formatted output, based on the knowledge of the value that
is about to be printed.  For example, when used to format integers, the
buffer is usually created as

char buf[] = "18446744073709551615UL";

which is the string representation of ULONG_MAX, or anything like that.

In general, I concur a more consistent and reliable usage of that
function should occur throughout the code.

p.




Ing. Pierangelo Masarati
Responsabile Open Solution
OpenLDAP Core Team

SysNet s.n.c.
Via Dossi, 8 - 27100 Pavia - ITALIA
http://www.sys-net.it
------------------------------------------
Office:   +39.02.23998309          
Mobile:   +39.333.4963172
Email:    pierangelo.masarati@sys-net.it
------------------------------------------



Followup 2

Download message
From: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
Date: Sun, 2 Apr 2006 18:03:42 +0200
To: Pierangelo Masarati <ando@sys-net.it>
Cc: openldap-its@openldap.org
Subject: Re: (ITS#4467) snprintf is consistenly used wrongly
Pierangelo Masarati writes:
> In many cases, especially when used to compute the length of a berval,
> snprintf is used under the assumption the buffer is large enough to
> contain the formatted output, based on the knowledge of the value that
> is about to be printed.  For example, when used to format integers,
> the buffer is usually created as
>
> char buf[] = "18446744073709551615UL";
>
> which is the string representation of ULONG_MAX, or anything like that.

Ah, so that's what these weird string constants are for.  That's wrong
too - whether or not that's big enough for ULONG_MAX depends on the
width of unsigned long.

We can use something like this.
Not sure if ldap_pvt or some other file is the best place for the macro:

ldap_pvt.h:

#include <limits.h>

/* Buffer space for sign, decimal digits and \0. Note: log10(2) < 146/485. */
#define LDAP_PVT_INTTYPE_CHARS(type) (((sizeof(type)*CHAR_BIT-1)*146)/485 + 3)

(Did I post that before?  Or was that to some other project?)

whatever.c:

  char buf[LDAP_PVT_INTTYPE_CHARS(unsigned long)];

Also, I think any remaining 'char buf[] = "unused text"; code should
be changed to
  char buf[sizeof("whatever")];
so that one can tell from reading the code that the initial contents
is irrelevant.  (As long as it's irrelevant even at failure :-)
I've been staring at some of that code and wondered WTF was going on.

-- 
Hallvard



Followup 3

Download message
Date: Sat, 29 Sep 2007 16:25:58 +0200
From: Pierangelo Masarati <ando@sys-net.it>
To: h.b.furuseth@usit.uio.no
CC: openldap-its@OpenLDAP.org
Subject: Re: (ITS#4467) snprintf is consistenly used wrongly
h.b.furuseth@usit.uio.no wrote:

> OpenLDAP is full of code like
>   ptr += snprintf( ptr, ... );
> and
>   bv.bv_len = snprintf( ... );
>   <use bv>;

Let me add that there seem to be occasional improper uses of sprintf()
too (usually in marginal code, fortunately).

p.



Ing. Pierangelo Masarati
OpenLDAP Core Team

SysNet s.r.l.
via Dossi, 8 - 27100 Pavia - ITALIA
http://www.sys-net.it
---------------------------------------
Office:  +39 02 23998309
Mobile:  +39 333 4963172
Email:   pierangelo.masarati@sys-net.it
---------------------------------------



Up to top level
Build   Contrib   Development   Documentation   Historical   Incoming   Software Bugs   Software Enhancements   Web  

Logged in as guest


The OpenLDAP Issue Tracking System uses a hacked version of JitterBug

______________
© Copyright 2013, OpenLDAP Foundation, info@OpenLDAP.org