contrib/ buffer paranoia

Neil Conway dijo:

The attached patch changes most of the usages of sprintf() to
snprintf() in contrib/. I didn't touch the places where pointer
arithmatic was being used, or other areas where the fix wasn't
trivial. I would think that few, if any, of the usages of sprintf()
were actually exploitable, but it's probably better to be paranoid...

Unless anyone sees a problem, please apply.

I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11
(snprintf counts the \0 at the end).

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Coge la flor que hoy nace alegre, ufana. Qui�n sabe si nacera otra ma�ana?"

Alvaro Herrera <alvherre@atentus.com> writes:

I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11
(snprintf counts the \0 at the end).

Yes, but so does the array declaration itself: a char[10] can hold at
most 9 characters plus the '\0' terminator. I think the original code
is buggy: if the author wants to store 10 characters plus a terminator
in the array, it should be declared as a char[11]. Using snprintf() of
length 11 with a char[10] would allow for a one-character overrun.

(Or did I not drink enough coffee this morning? :-) )

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

Neil Conway dijo:

Alvaro Herrera <alvherre@atentus.com> writes:

I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11
(snprintf counts the \0 at the end).

Yes, but so does the array declaration itself: a char[10] can hold at
most 9 characters plus the '\0' terminator. I think the original code
is buggy: if the author wants to store 10 characters plus a terminator
in the array, it should be declared as a char[11]. Using snprintf() of
length 11 with a char[10] would allow for a one-character overrun.

I agree. Maybe it worked out of pure luck (or some alignment magic).
But while you're at it, you can as well correct the bug.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)

Alvaro Herrera <alvherre@atentus.com> writes:

Neil Conway dijo:

Alvaro Herrera <alvherre@atentus.com> writes:

I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11
(snprintf counts the \0 at the end).

Yes, but so does the array declaration itself: a char[10] can hold at
most 9 characters plus the '\0' terminator. I think the original code
is buggy: if the author wants to store 10 characters plus a terminator
in the array, it should be declared as a char[11]. Using snprintf() of
length 11 with a char[10] would allow for a one-character overrun.

I agree. Maybe it worked out of pure luck (or some alignment magic).
But while you're at it, you can as well correct the bug.

Ok, a revised patch is attached that fixes the off-by-one bug in
dbase/dbf2pg.c

Thanks for the code review.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Neil Conway wrote:

Alvaro Herrera <alvherre@atentus.com> writes:

Neil Conway dijo:

Alvaro Herrera <alvherre@atentus.com> writes:

I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11
(snprintf counts the \0 at the end).

Yes, but so does the array declaration itself: a char[10] can hold at
most 9 characters plus the '\0' terminator. I think the original code
is buggy: if the author wants to store 10 characters plus a terminator
in the array, it should be declared as a char[11]. Using snprintf() of
length 11 with a char[10] would allow for a one-character overrun.

I agree. Maybe it worked out of pure luck (or some alignment magic).
But while you're at it, you can as well correct the bug.

Ok, a revised patch is attached that fixes the off-by-one bug in
dbase/dbf2pg.c

Thanks for the code review.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Patch applied. Thanks.

---------------------------------------------------------------------------

Neil Conway wrote:

Alvaro Herrera <alvherre@atentus.com> writes:

Neil Conway dijo:

Alvaro Herrera <alvherre@atentus.com> writes:

I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11
(snprintf counts the \0 at the end).

Yes, but so does the array declaration itself: a char[10] can hold at
most 9 characters plus the '\0' terminator. I think the original code
is buggy: if the author wants to store 10 characters plus a terminator
in the array, it should be declared as a char[11]. Using snprintf() of
length 11 with a char[10] would allow for a one-character overrun.

I agree. Maybe it worked out of pure luck (or some alignment magic).
But while you're at it, you can as well correct the bug.

Ok, a revised patch is attached that fixes the off-by-one bug in
dbase/dbf2pg.c

Thanks for the code review.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073