Re: [systemd-devel] [PATCH] systemd-bootchart: Prevent closing random file descriptors
On Sun, Mar 29, 2015 at 5:17 PM, Daniel Mack wrote:
> On 03/29/2015 03:04 PM, Alexander Sverdlin wrote:
>> On 29/03/15 13:44, Daniel Mack wrote:
@@ -184,6 +185,7 @@ vmstat_next:
> n = pread(schedstat, buf, sizeof(buf) - 1, 0); if (n <= 0) {
> close(schedstat); +schedstat = 0;
>>> Note that 0 is a valid file descriptor number. You should really
>>> rather reset the variables to -1 and check for '>= 0'. This applies
>>> to all hunks of this patch, which also needs a rebase onto the
>>> current git HEAD.
>>
>> I believe, it was HEAD as of time of patch submission, but I can of
>> course rebase it once again. Regarding 0: everywhere in the program
>> it relies on the fact that newly allocated memory is zeroed and files
>> are only opened if the corresponding file descriptor field of a
>> structure is 0. So do you propose to change the logic everywhere
>> where the files are opened?
>
> I see. As that code doesn't close stdin, 0 can't be returned by any
> open*(), so that's not a real issues, but all code should still be
> written in a way that it treats 0 as valid descriptor. So we need to
> explicitly initialize fd variables to -1 after new0(), and refactor code
> to where necessary.
Right, we rely on uninitialized fds to be negative. Even when it is
not commonly used, the _cleanup_close_ logic and other commonly
used fd handling functions, which should probably used in bootchart
too, rely on it:
http://cgit.freedesktop.org/systemd/systemd/tree/src/shared/util.c#n272
Kay
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] systemd-bootchart: Prevent closing random file descriptors
Hi Alexander,
On 03/29/2015 03:04 PM, Alexander Sverdlin wrote:
> On 29/03/15 13:44, Daniel Mack wrote:
>>> @@ -184,6 +185,7 @@ vmstat_next:
n = pread(schedstat, buf, sizeof(buf) - 1, 0); if (n <= 0) {
close(schedstat); +schedstat = 0;
>> Note that 0 is a valid file descriptor number. You should really
>> rather reset the variables to -1 and check for '>= 0'. This applies
>> to all hunks of this patch, which also needs a rebase onto the
>> current git HEAD.
>
> I believe, it was HEAD as of time of patch submission, but I can of
> course rebase it once again. Regarding 0: everywhere in the program
> it relies on the fact that newly allocated memory is zeroed and files
> are only opened if the corresponding file descriptor field of a
> structure is 0. So do you propose to change the logic everywhere
> where the files are opened?
I see. As that code doesn't close stdin, 0 can't be returned by any
open*(), so that's not a real issues, but all code should still be
written in a way that it treats 0 as valid descriptor. So we need to
explicitly initialize fd variables to -1 after new0(), and refactor code
to where necessary.
Thanks,
Daniel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] systemd-bootchart: Prevent closing random file descriptors
Hello Daniel,
On 29/03/15 13:44, Daniel Mack wrote:
@@ -184,6 +185,7 @@ vmstat_next:
> n = pread(schedstat, buf, sizeof(buf) - 1, 0);
> if (n <= 0) {
> close(schedstat);
>+schedstat = 0;
Note that 0 is a valid file descriptor number. You should really rather
reset the variables to -1 and check for '>= 0'. This applies to all
hunks of this patch, which also needs a rebase onto the current git HEAD.
I believe, it was HEAD as of time of patch submission, but I can of course
rebase it once again.
Regarding 0: everywhere in the program it relies on the fact that newly
allocated memory is zeroed
and files are only opened if the corresponding file descriptor field of a
structure is 0. So do you
propose to change the logic everywhere where the files are opened?
Alexander.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] systemd-bootchart: Prevent closing random file descriptors
On 03/29/2015 03:13 AM, Alexander Sverdlin wrote:
> Fix it by zeroing all the closed descriptors immediately, this would repair
> existing caching of open files and clean-up strategy.
>
> The fix is important even with CONFIG_SCHED_DEBUG option enabled, because very
> first failure to open /proc//* because process exited will result in some
> other victim descriptor being closed later and will therefore disturb the
> whole
> collected statistics.
> ---
> src/bootchart/store.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/src/bootchart/store.c b/src/bootchart/store.c
> index dfa681f..8525f62 100644
> --- a/src/bootchart/store.c
> +++ b/src/bootchart/store.c
> @@ -152,6 +152,7 @@ void log_sample(int sample, struct list_sample_data
> **ptr) {
> n = pread(vmstat, buf, sizeof(buf) - 1, 0);
> if (n <= 0) {
> close(vmstat);
> +vmstat = 0;
> return;
> }
> buf[n] = '\0';
> @@ -184,6 +185,7 @@ vmstat_next:
> n = pread(schedstat, buf, sizeof(buf) - 1, 0);
> if (n <= 0) {
> close(schedstat);
> +schedstat = 0;
Note that 0 is a valid file descriptor number. You should really rather
reset the variables to -1 and check for '>= 0'. This applies to all
hunks of this patch, which also needs a rebase onto the current git HEAD.
Thanks,
Daniel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
