История изменений
Исправление firkax, (текущая версия) :
Там прямо перед вызовом функции вонзили проверку на переполнение.
Мда и всё равно сделали плохо. Надо было в комментарии указать, что, поскольку alignment это степень двойки (это видно из другого кода, который в diff-е не участвует), он не может стать больше чем SIZE_MAX-MINSIZE и соответственно не надо проверять на переполнение это вычитание. Судя по отсутствию комментария (но при этом «Check for overflow.» они вписать не забыли) - вполне можно предположить что они об этом просто не подумали и, не будь такого внешнего ограничения на alignment, получили бы всё то же переполнение.
Ну и сам способ имеющейся проверки плохой. Он, хоть и работает, но использует лишние сущности (усложняет восприятие кода) и такты процессора. Правильно было бы так:
+ size_t aligned_nb;
.....
+ aligned_nb = nb + alignment + MINSIZE;
+ if (aligned_nb <= nb)
+ {
+ __set_errno (ENOMEM);
+ return 0;
+ }
+
/* Call malloc with worst case padding to hit alignment. */
- m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));
+ m = (char *) (_int_malloc (av, aligned_nb));
И вот такие if-ы должны быть не каким-то отдельным действием в логике работы функции, которое они как «check for overflow» расписали, а автоматически (т.е. не требуя очевидных объяснений) сопровождать любую арифметику, если нет 100% уверенности что переполнения тут быть не может. Если есть опасение что может переполниться alignment+MINSIZE - их надо аналогично проверить.
Скрин из отчёта coverity. Как видишь ничего он не нашёл здесь.
Очень плохо и для него, и для всех остальных, кто не нашёл. Найти было проще простого, оно в глаза бросается при беглом просмотре кода. Не нашёл видимо потому, что подобные дефекты - норма для толп плохих кодеров, и анализаторы не хотят об этом спамить. Может быть даже авторы анализаторов считают это нормой.
Во первых все эти ошибки легко не делать.
Почему тогда их постоянно находят в крупных проектах? Тысячи глаз же.
А, ну и да: программист, который это (malloc с формулой из сложений, умножений и/или вычитаний в качестве аргумента (деления можно), к которой рядом нет доказательства её безопасности) написал - профнепригоден, по крайней мере в сфере системного программирования. Это действительно легко НЕ делать.
Исправление firkax, :
Там прямо перед вызовом функции вонзили проверку на переполнение.
Мда и всё равно сделали плохо. Надо было в комментарии указать, что, поскольку alignment это степень двойки (это видно из другого кода, который в diff-е не участвует), он не может стать больше чем SIZE_MAX-MINSIZE и соответственно не надо проверять на переполнение это вычитание. Судя по отсутствию комментария (но при этом «Check for overflow.» они вписать не забыли) - вполне можно предположить что они об этом просто не подумали и, не будь такого внешнего ограничения на alignment, получили бы всё то же переполнение.
Ну и сам способ имеющейся проверки плохой. Он, хоть и работает, но использует лишние сущности (усложняет восприятие кода) и такты процессора. Правильно было бы так:
+ size_t aligned_nb;
.....
+ aligned_nb = nb + alignment + MINSIZE;
+ if (aligned_nb <= nb)
+ {
+ __set_errno (ENOMEM);
+ return 0;
+ }
+
/* Call malloc with worst case padding to hit alignment. */
- m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));
+ m = (char *) (_int_malloc (av, aligned_nb));
И вот такие if-ы должны быть не каким-то отдельным действием в логике работы функции, которое они как «check for overflow» расписали, а автоматически (т.е. не требуя очевидных объяснений) сопровождать любую арифметику, если нет 100% уверенности что переполнения тут быть не может. Если есть опасение что может переполниться alignment+MINSIZE - их надо аналогично проверить.
Скрин из отчёта coverity. Как видишь ничего он не нашёл здесь.
Очень плохо и для него, и для всех остальных, кто не нашёл. Найти было проще простого, оно в глаза бросается при беглом просмотре кода. Не нашёл видимо потому, что подобные дефекты - норма для толп плохих кодеров, и анализаторы не хотят об этом спамить. Может быть даже авторы анализаторов считают это нормой.
Во первых все эти ошибки легко не делать.
Почему тогда их постоянно находят в крупных проектах? Тысячи глаз же.
А, ну и да: программист, который это (malloc с формулой в качестве аргумента, к которой рядом нет доказательства её безопасности) написал - профнепригоден, по крайней мере в сфере системного программирования. Это действительно легко НЕ делать.
Исправление firkax, :
Там прямо перед вызовом функции вонзили проверку на переполнение.
Мда и всё равно сделали плохо. Надо было в комментарии указать, что, поскольку alignment это степень двойки (это видно из другого кода, который в diff-е не участвует), он не может стать больше чем SIZE_MAX-MINSIZE и соответственно не надо проверять на переполнение это вычитание. Судя по отсутствию комментария (но при этом «Check for overflow.» они вписать не забыли) - вполне можно предположить что они об этом просто не подумали и, не будь такого внешнего ограничения на alignment, получили бы всё то же переполнение.
Ну и сам способ имеющейся проверки плохой. Он, хоть и работает, но использует лишние сущности (усложняет восприятие кода) и такты процессора. Правильно было бы так:
+ size_t aligned_nb;
.....
+ aligned_nb = nb + alignment + MINSIZE;
+ if (aligned_nb <= nb)
+ {
+ __set_errno (ENOMEM);
+ return 0;
+ }
+
/* Call malloc with worst case padding to hit alignment. */
- m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));
+ m = (char *) (_int_malloc (av, aligned_nb));
И вот такие if-ы должны быть не каким-то отдельным действием в логике работы функции, которое они как «check for overflow» расписали, а автоматически (т.е. не требуя очевидных объяснений) сопровождать любую арифметику, если нет 100% уверенности что переполнения тут быть не может. Если есть опасение что может переполниться alignment+MINSIZE - их надо аналогично проверить.
Скрин из отчёта coverity. Как видишь ничего он не нашёл здесь.
Очень плохо и для него, и для всех остальных, кто не нашёл. Найти было проще простого, оно в глаза бросается при беглом просмотре кода. Не нашёл видимо потому, что подобные дефекты - норма для толп плохих кодеров, и анализаторы не хотят об этом спамить. Может быть даже авторы анализаторов считают это нормой.
Во первых все эти ошибки легко не делать.
Почему тогда их постоянно находят в крупных проектах? Тысячи глаз же.
А, ну и да: программист, который это (malloc с формулой в качестве аргумента, к которой рядом нет доказательства её безопасности) написал - профнепригоден, по крайней мере в сфере системного программирования.
Исправление firkax, :
Там прямо перед вызовом функции вонзили проверку на переполнение.
Мда и всё равно сделали плохо. Надо было в комментарии указать, что, поскольку alignment это степень двойки (это видно из другого кода, который в diff-е не участвует), он не может стать больше чем SIZE_MAX-MINSIZE и соответственно не надо проверять на переполнение это вычитание. Судя по отсутствию комментария (но при этом «Check for overflow.» они вписать не забыли) - вполне можно предположить что они об этом просто не подумали и, не будь такого внешнего ограничения на alignment, получили бы всё то же переполнение.
Ну и сам способ имеющейся проверки плохой. Он, хоть и работает, но использует лишние сущности (усложняет восприятие кода) и такты процессора. Правильно было бы так:
+ size_t aligned_nb;
.....
+ aligned_nb = nb + alignment + MINSIZE;
+ if (aligned_nb <= nb)
+ {
+ __set_errno (ENOMEM);
+ return 0;
+ }
+
/* Call malloc with worst case padding to hit alignment. */
- m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));
+ m = (char *) (_int_malloc (av, aligned_nb));
И вот такие if-ы должны быть не каким-то отдельным действием в логике работы функции, которое они как «check for overflow» расписали, а автоматически (т.е. не требуя очевидных объяснений) сопровождать любую арифметику, если нет 100% уверенности что переполнения тут быть не может. Если есть опасение что может переполниться alignment+MINSIZE - их надо аналогично проверить.
Скрин из отчёта coverity. Как видишь ничего он не нашёл здесь.
Очень плохо и для него, и для всех остальных, кто не нашёл. Найти было проще простого, оно в глаза бросается при беглом просмотре кода. Не нашёл видимо потому, что подобные дефекты - норма для толп плохих кодеров, и анализаторы не хотят об этом спамить. Может быть даже авторы анализаторов считают это нормой.
Исходная версия firkax, :
Там прямо перед вызовом функции вонзили проверку на переполнение.
Мда и всё равно сделали плохо. Надо было в комментарии указать, что, поскольку alignment это степень двойки (это видно из другого кода, который в diff-е не участвует), он не может стать больше чем SIZE_MAX-MINSIZE и соответственно не надо проверять на переполнение это вычитание. Судя по отсутствию комментария (но при этом «Check for overflow.» они вписать не забыли) - вполне можно предположить что они об этом просто не подумали и, не будь такого внешнего ограничения на alignment, получили бы всё то же переполнение.
Ну и сам способ имеющейся проверки плохой. Он, хоть и работает, но использует лишние сущности (усложняет восприятие кода) и такты процессора. Правильно было бы так:
+ size_t aligned_nb;
.....
+ aligned_nb = nb + alignment + MINSIZE;
+ if (aligned_nb <= nb)
+ {
+ __set_errno (ENOMEM);
+ return 0;
+ }
+
/* Call malloc with worst case padding to hit alignment. */
- m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));
+ m = (char *) (_int_malloc (av, aligned_nb));
И вот такие if-ы должны быть не каким-то отдельным действием в логике работы функции, которое они как «check for overflow» расписали, а автоматически (т.е. не требуя очевидных объяснений) сопровождать любую арифметику, если нет 100% уверенности что переполнения тут быть не может. Если есть опасение что может переполниться alignment+MINSIZE - их надо аналогично проверить.