Re: [devel] [git update] packages/katrin: heads/master

Re: [devel] [git update] packages/katrin: heads/master

[Slava Semushin]

2007/6/19, Denis Klimov <zver / altlinux.org>:
[...]
> diff --git a/src/tc/katrin-tc.c b/src/tc/katrin-tc.c
[...]
> +if (argv[1]!=NULL && (strcmp(argv[1],"remove")==0 || strcmp(argv[1],"reload")==0))
> +{
> +       char *iface = strdup("eth0");
> +       char *allrate = strdup("100mbit");

strdup() выделяет новую память с помощью malloc(). Это значит, что её
можно (и нужно) освободить во избежание утечек.

[...]
> +       //open tmp script file
> +       char prog[] = "/tmp/katrin-tc-tmp";
> +       char path[] = "/tmp";
[...]

Эти строчки несколько коробят глаз. Я не знаю всех тонкостей вашей
задачи, но кажется, что использование mkstemp(3) тут было бы к месту.

[...]
> +       sprintf(command,"tc qdisc del dev %s root handle 1: htb;\n",iface);

Многочисленное использование sprintf() и strcpy() в коде наводит на
мысль, что пользователям вашего билинга может повезти :)

[...]
> +       fclose(fh);

Вот здесь (если не ошибаюсь) можно и память освободить:

+    free(iface);
+    free(allrate);

[...]

JFYI.


-- 
+ Slava Semushin | slava.semushin @ gmail.com
+ ALT Linux Team | php-coder @ altlinux.ru

Re: [devel] [git update] packages/katrin: heads/master

[Денис Климов]

Здравствуйте,
Благодарю за дельные советы.

> [...]
>> +       sprintf(command,"tc qdisc del dev %s root handle 1: htb;\n",iface);

> Многочисленное использование sprintf() и strcpy() в коде наводит на
> мысль, что пользователям вашего билинга может повезти :)

есть альтернатива когда надо подставить в строку параметры?



-- 
C наилучшими пожеланиями,
 Климов Денис
kliden@km.ru

=============================================================
      Thursday, June 21, 2007, 6:15:03 PM, было:
-------------------------------------------------------------

> 2007/6/19, Denis Klimov <zver / altlinux.org>:
> [...]
>> diff --git a/src/tc/katrin-tc.c b/src/tc/katrin-tc.c
> [...]
>> +if (argv[1]!=NULL && (strcmp(argv[1],"remove")==0 || strcmp(argv[1],"reload")==0))
>> +{
>> +       char *iface = strdup("eth0");
>> +       char *allrate = strdup("100mbit");

> strdup() выделяет новую память с помощью malloc(). Это значит, что её
> можно (и нужно) освободить во избежание утечек.

> [...]
>> +       //open tmp script file
>> +       char prog[] = "/tmp/katrin-tc-tmp";
>> +       char path[] = "/tmp";
> [...]

> Эти строчки несколько коробят глаз. Я не знаю всех тонкостей вашей
> задачи, но кажется, что использование mkstemp(3) тут было бы к месту.

> [...]
>> +       sprintf(command,"tc qdisc del dev %s root handle 1: htb;\n",iface);

> Многочисленное использование sprintf() и strcpy() в коде наводит на
> мысль, что пользователям вашего билинга может повезти :)

> [...]
>> +       fclose(fh);

> Вот здесь (если не ошибаюсь) можно и память освободить:

> +    free(iface);
> +    free(allrate);

> [...]

> JFYI.

Re: [devel] [git update] packages/katrin: heads/master

[Slava Semushin]

2007/6/22, Денис Климов <kliden / km.ru>:
> Здравствуйте,
> Благодарю за дельные советы.

Рад, что мог быть вам полезен :)

> > [...]
> >> +       sprintf(command,"tc qdisc del dev %s root handle 1: htb;\n",iface);
>
> > Многочисленное использование sprintf() и strcpy() в коде наводит на
> > мысль, что пользователям вашего билинга может повезти :)
>
> есть альтернатива когда надо подставить в строку параметры?
[...]

Альтернатив не знаю, но думаю, что а) жесткая проверка параметров б)
использование snprintf() вместо sprintf() и str[nl]cpy() вместо
strcpy() должно существенно помочь вашему коду и, как мне думается,
обезопасить от многих простейших аттак.

P.S. В Сизифе, кстати, есть flawfinder (чуток устаревший, правда, но
багу и повесил :) ), который может указывать на ф-ции, которые
небезопасно использовать. Во всём на него полагаться я бы не стал, а
вот прислушаться стОит :)

-- 
+ Slava Semushin | slava.semushin @ gmail.com
+ ALT Linux Team | php-coder @ altlinux.ru

Re: [devel] [git update] packages/katrin: heads/master

[Денис Смирнов]

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

On Thu, Jun 21, 2007 at 07:15:03PM +0700, Slava Semushin wrote:
>> diff --git a/src/tc/katrin-tc.c b/src/tc/katrin-tc.c
>> +if (argv[1]!=NULL && (strcmp(argv[1],"remove")==0 || strcmp(argv[1],"reload")==0))
>> +{
>> +       char *iface = strdup("eth0");
>> +       char *allrate = strdup("100mbit");
SS> strdup() выделяет новую память с помощью malloc(). Это значит, что её
SS> можно (и нужно) освободить во избежание утечек.

Добавлю, что если строки столь небольшие, то возможно имеет смысл
использовать strdupa. Хотя не читая кода далее мне непонятно в связи с чем
используется подобное, вместо указателя на константу.

>> +       //open tmp script file
>> +       char prog[] = "/tmp/katrin-tc-tmp";
>> +       char path[] = "/tmp";
SS> Эти строчки несколько коробят глаз. Я не знаю всех тонкостей вашей
SS> задачи, но кажется, что использование mkstemp(3) тут было бы к месту.

Угу. Создание временных файлов со статическими именами неприемлимо.

>> +       sprintf(command,"tc qdisc del dev %s root handle 1: htb;\n",iface);
SS> Многочисленное использование sprintf() и strcpy() в коде наводит на
SS> мысль, что пользователям вашего билинга может повезти :)

Кстати есть ещё asprintf. Наиболее удобный вариант для подобных
применений, IMHO.

>> +       fclose(fh);
SS> Вот здесь (если не ошибаюсь) можно и память освободить:
SS> +    free(iface);
SS> +    free(allrate);

Если использовать strdupa/asprintf то можно (и нужно) не освобождать. Ибо
память выделяется в стеке.

Эти функции не шибко-то переносимы, но потрясающе удобны.

-- 
С уважением, Денис

http://freesource.info
----------------------------------------------------------------------------
Хотел предложить воспользоваться bushbug, но он оказался сломанным.
		-- ldv in sisyphus@

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [devel] [git update] packages/katrin: heads/master

[Andrey Rahmatullin]

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

On Sat, Jun 23, 2007 at 06:40:46PM +0400, Денис Смирнов wrote:
> Если использовать strdupa/asprintf то можно (и нужно) не освобождать. Ибо
> память выделяется в стеке.
А хренли в мане This pointer should be passed to free(3) ?

-- 
WBR, wRAR (ALT Linux Team)
Powered by the ALT Linux fortune(8):

Собственно говоря, пакет glibc предназначен для того, чтобы не потерять
разные части glibc, если только вы не знаете заранее, что они не все вам
нужны.
		-- ldv in community@

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [devel] [git update] packages/katrin: heads/master

[Денис Смирнов]

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

On Sat, Jun 23, 2007 at 08:42:24PM +0600, Andrey Rahmatullin wrote:
 AR> On Sat, Jun 23, 2007 at 06:40:46PM +0400, Денис Смирнов wrote:
>> Если использовать strdupa/asprintf то можно (и нужно) не освобождать. Ибо
>> память выделяется в стеке.
AR> А хренли в мане This pointer should be passed to free(3) ?

Извиняюсь что ввел в заблуждение по поводу asprintf.

А вот для strdupa:
strdupa()  and  strndupa()  are  similar,  but use alloca(3) to allocate
the buffer.  They are only available when using the GNU GCC suite, and
suffer from the same limitations described in alloca(3).

Ну а что alloca выделяет память на стеке (со всеми преимуществами и
недостатками этого подхода) факт общеизвестный.

-- 
С уважением, Денис

http://freesource.info
----------------------------------------------------------------------------
<drF_ckoff> воткнуть чтоль обратно /^unknown$/ 550 "V bobruysk!"

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]