Skip to content

Added warning about uninitialized memory. #29

Closed
wants to merge 4 commits into from

2 participants

@chrisdew

Added warning about uninitialized memory. The example code does not make it clear that the "options" variable must be initialized to zero before use.

This helped me write a bug which took a few minutes to find with the help of Valgrind.

@nikhilm nikhilm commented on the diff Feb 13, 2013
source/processes.rst
@@ -31,6 +31,11 @@ exits. This is achieved using ``uv_spawn``.
:lines: 5-7,13-
:emphasize-lines: 11,13-17
+Note: ``options``, being a global variable, is implicitly initialized with
@nikhilm
Owner
nikhilm added a note Feb 13, 2013

Please use reStructuredText NOTE syntax

.. NOTE::

    note text follows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikhilm nikhilm commented on the diff Feb 13, 2013
source/processes.rst
@@ -31,6 +31,11 @@ exits. This is achieved using ``uv_spawn``.
:lines: 5-7,13-
:emphasize-lines: 11,13-17
+Note: ``options``, being a global variable, is implicitly initialized with
+zeros. If you change ``options`` to a local variable, memory corruption
+will follow (thanks Valgrind for saving me a few hours!). You can use
@nikhilm
Owner
nikhilm added a note Feb 13, 2013

remove parens about Valgrind :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikhilm nikhilm commented on the diff Feb 13, 2013
source/processes.rst
@@ -31,6 +31,11 @@ exits. This is achieved using ``uv_spawn``.
:lines: 5-7,13-
:emphasize-lines: 11,13-17
+Note: ``options``, being a global variable, is implicitly initialized with
+zeros. If you change ``options`` to a local variable, memory corruption
+will follow (thanks Valgrind for saving me a few hours!). You can use
+``memset`` to initialize a local variable of type ``uv_process_options_t``.
+
@nikhilm
Owner
nikhilm added a note Feb 13, 2013

Rather than having this note, can you just change spawn/main.c itself to make options local to main and use

uv_process_options_t options = {0};

syntax instead of memset.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nikhilm nikhilm added a commit that closed this pull request Apr 3, 2013
@chrisdew chrisdew Added note about options initialization. Fixes #29.
Update source/processes.rst

edited note

Update source/processes.rst

edited note

Update source/processes.rst

edited note
e8a5efe
@nikhilm nikhilm closed this in e8a5efe Apr 3, 2013
@nikhilm nikhilm added a commit that referenced this pull request Apr 3, 2013
@nikhilm Generated gh-pages for commit e8a5efe (origin/master, master)
Author: Chris Dew <cmsdew@gmail.com>

    Added note about options initialization. Fixes #29.
3452895
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.