upstart

Merge lp:~jamesodhunt/upstart/bug-1222705-reprise into lp:upstart

  1. bug-1222705-reprise
  2. Merge into trunk
Proposed by James Hunt
Status: Merged
Merged at revision: 1656
Proposed branch: lp:~jamesodhunt/upstart/bug-1222705-reprise
Merge into: lp:upstart
Diff against target: 304 lines (+45/-86)
3 files modified
ChangeLog (+8/-0)
init/environ.c (+12/-14)
init/tests/test_environ.c (+25/-72)
To merge this branch: bzr merge lp:~jamesodhunt/upstart/bug-1222705-reprise
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Dimitri John Ledkov Needs Information
Steve Langasek please review memoves against (potentially) nih_alloced char** array Pending
Review via email: mp+226983@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Dimitri John Ledkov (xnox) wrote :

See inline comment about memove call.

review: Needs Information
Revision history for this message
Colin Watson (cjwatson) wrote :

memcpy is unsafe for that, but the entire point of memmove is to be safe in this situation.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

memmove's are all good. Not sure if it's safe to use them against nih_alloced objects, given they have NihAllocCtx after them. Someone else should review this bit. See also the inline comment.

review: Needs Information
lp:~jamesodhunt/upstart/bug-1222705-reprise updated
1657. By James Hunt

* init/environ.c: environ_remove(): Shrink memory to fit what we're
  actually using.
* init/tests/test_environ.c: test_remove(): Remove now-invalid ENOMEM
  checks.

Revision history for this message
James Hunt (jamesodhunt) wrote :

It's safe because all the environ_*() calls operate on the result of nih_str_array_new(). That function will return a block of memory big enough to hold a single 'char *' pointer, and set it to NULL. The NihAllocCtx is situated _below_ the return value of nih_str_array_new() in memory and that single NihAllocCtx will be used to keep track of the size and child allocations of the entire array (should it grow bigger than its current size of zero (the terminator is not counted)).

Subsequent to nih_str_array_new() being called, that zero-sized array will be increased in size by calls to environ_add(), which in turn calls nih_str_array_addp(). That latter function then calls nih_realloc() to increase the size of the single NihAllocCtx previously mentioned.

So, if the array contains pointers to 10 env vars, it has size 11 (10 + 1 for the NULL terminator), and has a single NihAllocCtx just before the zeroth element. So it's actual byte size is:

  sizeof (NihAllocCtx) + (sizeof (char *) * (number_of_elements + 1))

As such, it is safe to call memmove to shuffle those pointers since they are the user-accessible way to reference the strings the array points to - Nih keeps track itself using the NihAllocCtx object.

I've added a call to nih_relloc() to compact the size of the array (from NIH's perspective) as it is wasteful (but not actually incorrect) not to call nih_realloc() and leave 'sizeof (char *)' bytes lying around until the array eventually gets deallocated.

Revision history for this message
Colin Watson (cjwatson) wrote :

I agree with your analysis re NihAllocCtx. This looks right, and the form of the new code matches a similar section from environ_add, so that's good. Just one style comment.

review: Approve
Revision history for this message
James Hunt (jamesodhunt) wrote :

To be convinced this fix works:

$ mkdir /tmp/conf /tmp/log
$ cat >>/tmp/conf/bug-1222705.conf<<EOT
start on startup

exec sleep 999
post-stop exec initctl unset-env wibble
EOT
$ gdb --args init/init --confdir /tmp/conf --logdir /tmp/log --debug
(gdb) run

In separate window:

$ initctl list-sessions
$ UPSTART_SESSION=unix:abstract=/com/ubuntu/upstart-session/$uid/$pid restart bug-1222705

lp:~jamesodhunt/upstart/bug-1222705-reprise updated
1658. By James Hunt

* init/environ.c: environ_remove(): Style tweak courtesy of cjwatson's
  review.

Revision history for this message
James Hunt (jamesodhunt) wrote :

Thanks - style tweak applied.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'ChangeLog'
--- ChangeLog 2014-07-11 21:32:38 +0000
+++ ChangeLog 2014-07-16 15:55:55 +0000
@@ -1,3 +1,11 @@
12014-07-16 James Hunt <james.hunt@ubuntu.com>
2
3 * init/environ.c: environ_remove():
4 - Ensure removed entry is deref'd (LP: #1222705).
5 - Avoid recreating array.
6 * init/tests/test_environ.c: test_remove(): Add checks to ensure
7 removed entry freed as expected.
8
12014-07-11 James Hunt <james.hunt@ubuntu.com>92014-07-11 James Hunt <james.hunt@ubuntu.com>
210
3 * NEWS: Release 1.1311 * NEWS: Release 1.13
412
=== modified file 'init/environ.c'
--- init/environ.c 2013-10-24 13:33:05 +0000
+++ init/environ.c 2014-07-16 15:55:55 +0000
@@ -176,9 +176,7 @@
176{176{
177 size_t _len;177 size_t _len;
178 size_t keylen;178 size_t keylen;
179 size_t new_len = 0;
180 char **e;179 char **e;
181 char **new_env;
182180
183 nih_assert (env);181 nih_assert (env);
184 nih_assert (str);182 nih_assert (str);
@@ -195,10 +193,6 @@
195 if (*len < 1)193 if (*len < 1)
196 return NULL;194 return NULL;
197195
198 new_env = nih_str_array_new (NULL);
199 if (! new_env)
200 return NULL;
201
202 for (e = *env; e && *e; e++) {196 for (e = *env; e && *e; e++) {
203 keylen = strcspn (*e, "=");197 keylen = strcspn (*e, "=");
204198
@@ -206,17 +200,21 @@
206 * name=value pair, or a bare name), so don't copy it to200 * name=value pair, or a bare name), so don't copy it to
207 * the new environment.201 * the new environment.
208 */202 */
209 if (! strncmp (str, *e, keylen))203 if (! strncmp (str, *e, keylen)) {
210 continue;204 nih_unref (*e, *env);
211205
212 if (! environ_add (&new_env, parent, &new_len, TRUE, *e))206 /* shuffle up the remaining entries */
213 return NULL;207 memmove (e, e + 1, (char *)(*env + *len) - (char *)e);
208
209 (*len)--;
210 }
214 }211 }
215212
216 *env = new_env;213 /* shrink amount of memory used */
217 *len = new_len;214 if (! nih_realloc (*env, parent, sizeof (**env) * (*len + 1)))
215 return NULL;
218216
219 return new_env;217 return *env;
220}218}
221219
222/**220/**
223221
=== modified file 'init/tests/test_environ.c'
--- init/tests/test_environ.c 2013-10-24 13:33:05 +0000
+++ init/tests/test_environ.c 2014-07-16 15:55:55 +0000
@@ -618,8 +618,10 @@
618void618void
619test_remove (void)619test_remove (void)
620{620{
621 char **env = NULL, **ret;621 char **env = NULL;
622 size_t len = 0;622 char *removed = NULL;
623 char **ret = NULL;
624 size_t len = 0;
623625
624 TEST_FUNCTION ("environ_remove");626 TEST_FUNCTION ("environ_remove");
625627
@@ -688,6 +690,9 @@
688 TEST_ALLOC_PARENT (env[0], env);690 TEST_ALLOC_PARENT (env[0], env);
689 TEST_ALLOC_SIZE (env[0], 8);691 TEST_ALLOC_SIZE (env[0], 8);
690 TEST_EQ_STR (env[0], "FOO=BAR");692 TEST_EQ_STR (env[0], "FOO=BAR");
693 removed = env[0];
694 TEST_FREE_TAG (removed);
695
691 TEST_EQ_P (env[1], NULL);696 TEST_EQ_P (env[1], NULL);
692 }697 }
693698
@@ -695,17 +700,6 @@
695700
696 if (test_alloc_failed) {701 if (test_alloc_failed) {
697 TEST_EQ_P (ret, NULL);702 TEST_EQ_P (ret, NULL);
698
699 TEST_EQ (len, 1);
700
701 TEST_ALLOC_PARENT (env[0], env);
702 TEST_ALLOC_SIZE (env[0], 8);
703
704 TEST_NE_P (env[0], NULL);
705 TEST_EQ_STR (env[0], "FOO=BAR");
706
707 TEST_EQ_P (env[1], NULL);
708
709 nih_free (env);703 nih_free (env);
710 continue;704 continue;
711 }705 }
@@ -713,6 +707,7 @@
713 TEST_NE_P (ret, NULL);707 TEST_NE_P (ret, NULL);
714 TEST_EQ (len, 0);708 TEST_EQ (len, 0);
715 TEST_EQ_P (env[0], NULL);709 TEST_EQ_P (env[0], NULL);
710 TEST_FREE (removed);
716711
717 nih_free (env);712 nih_free (env);
718 }713 }
@@ -730,6 +725,9 @@
730 TEST_ALLOC_PARENT (env[0], env);725 TEST_ALLOC_PARENT (env[0], env);
731 TEST_ALLOC_SIZE (env[0], 8);726 TEST_ALLOC_SIZE (env[0], 8);
732 TEST_EQ_STR (env[0], "FOO=BAR");727 TEST_EQ_STR (env[0], "FOO=BAR");
728 removed = env[0];
729 TEST_FREE_TAG (removed);
730
733 TEST_EQ_P (env[1], NULL);731 TEST_EQ_P (env[1], NULL);
734 }732 }
735733
@@ -737,17 +735,6 @@
737735
738 if (test_alloc_failed) {736 if (test_alloc_failed) {
739 TEST_EQ_P (ret, NULL);737 TEST_EQ_P (ret, NULL);
740
741 TEST_EQ (len, 1);
742
743 TEST_ALLOC_PARENT (env[0], env);
744 TEST_ALLOC_SIZE (env[0], 8);
745
746 TEST_NE_P (env[0], NULL);
747 TEST_EQ_STR (env[0], "FOO=BAR");
748
749 TEST_EQ_P (env[1], NULL);
750
751 nih_free (env);738 nih_free (env);
752 continue;739 continue;
753 }740 }
@@ -755,6 +742,7 @@
755 TEST_NE_P (ret, NULL);742 TEST_NE_P (ret, NULL);
756 TEST_EQ (len, 0);743 TEST_EQ (len, 0);
757 TEST_EQ_P (env[0], NULL);744 TEST_EQ_P (env[0], NULL);
745 TEST_FREE (removed);
758746
759 nih_free (env);747 nih_free (env);
760 }748 }
@@ -781,6 +769,8 @@
781 TEST_ALLOC_PARENT (env[0], env);769 TEST_ALLOC_PARENT (env[0], env);
782 TEST_ALLOC_SIZE (env[0], 8);770 TEST_ALLOC_SIZE (env[0], 8);
783 TEST_EQ_STR (env[0], "FOO=BAR");771 TEST_EQ_STR (env[0], "FOO=BAR");
772 removed = env[0];
773 TEST_FREE_TAG (removed);
784774
785 TEST_ALLOC_PARENT (env[1], env);775 TEST_ALLOC_PARENT (env[1], env);
786 TEST_ALLOC_SIZE (env[1], 8);776 TEST_ALLOC_SIZE (env[1], 8);
@@ -794,18 +784,6 @@
794784
795 if (test_alloc_failed) {785 if (test_alloc_failed) {
796 TEST_EQ_P (ret, NULL);786 TEST_EQ_P (ret, NULL);
797
798 TEST_EQ (len, 2);
799 TEST_ALLOC_PARENT (env[0], env);
800 TEST_ALLOC_SIZE (env[0], 8);
801 TEST_EQ_STR (env[0], "FOO=BAR");
802
803 TEST_ALLOC_PARENT (env[1], env);
804 TEST_ALLOC_SIZE (env[1], 8);
805 TEST_EQ_STR (env[1], "BAZ=QUX");
806
807 TEST_EQ_P (env[2], NULL);
808
809 nih_free (env);787 nih_free (env);
810 continue;788 continue;
811 }789 }
@@ -816,6 +794,7 @@
816 TEST_ALLOC_PARENT (env[0], env);794 TEST_ALLOC_PARENT (env[0], env);
817 TEST_ALLOC_SIZE (env[0], 8);795 TEST_ALLOC_SIZE (env[0], 8);
818 TEST_EQ_STR (env[0], "BAZ=QUX");796 TEST_EQ_STR (env[0], "BAZ=QUX");
797 TEST_FREE (removed);
819798
820 TEST_EQ_P (env[1], NULL);799 TEST_EQ_P (env[1], NULL);
821800
@@ -852,6 +831,8 @@
852 TEST_ALLOC_PARENT (env[0], env);831 TEST_ALLOC_PARENT (env[0], env);
853 TEST_ALLOC_SIZE (env[0], 8);832 TEST_ALLOC_SIZE (env[0], 8);
854 TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");833 TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");
834 removed = env[0];
835 TEST_FREE_TAG (removed);
855836
856 TEST_ALLOC_PARENT (env[1], env);837 TEST_ALLOC_PARENT (env[1], env);
857 TEST_ALLOC_SIZE (env[1], 8);838 TEST_ALLOC_SIZE (env[1], 8);
@@ -865,18 +846,6 @@
865846
866 if (test_alloc_failed) {847 if (test_alloc_failed) {
867 TEST_EQ_P (ret, NULL);848 TEST_EQ_P (ret, NULL);
868
869 TEST_EQ (len, 2);
870 TEST_ALLOC_PARENT (env[0], env);
871 TEST_ALLOC_SIZE (env[0], 8);
872 TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");
873
874 TEST_ALLOC_PARENT (env[1], env);
875 TEST_ALLOC_SIZE (env[1], 8);
876 TEST_EQ_STR (env[1], "BAZ=QUX");
877
878 TEST_EQ_P (env[2], NULL);
879
880 nih_free (env);849 nih_free (env);
881 continue;850 continue;
882 }851 }
@@ -887,6 +856,7 @@
887 TEST_ALLOC_PARENT (env[0], env);856 TEST_ALLOC_PARENT (env[0], env);
888 TEST_ALLOC_SIZE (env[0], 8);857 TEST_ALLOC_SIZE (env[0], 8);
889 TEST_EQ_STR (env[0], "BAZ=QUX");858 TEST_EQ_STR (env[0], "BAZ=QUX");
859 TEST_FREE (removed);
890860
891 TEST_EQ_P (env[1], NULL);861 TEST_EQ_P (env[1], NULL);
892862
@@ -921,6 +891,8 @@
921 TEST_ALLOC_PARENT (env[1], env);891 TEST_ALLOC_PARENT (env[1], env);
922 TEST_ALLOC_SIZE (env[1], 8);892 TEST_ALLOC_SIZE (env[1], 8);
923 TEST_EQ_STR (env[1], "BAZ=QUX");893 TEST_EQ_STR (env[1], "BAZ=QUX");
894 removed = env[1];
895 TEST_FREE_TAG (removed);
924896
925 TEST_EQ_P (env[2], NULL);897 TEST_EQ_P (env[2], NULL);
926 }898 }
@@ -930,18 +902,6 @@
930902
931 if (test_alloc_failed) {903 if (test_alloc_failed) {
932 TEST_EQ_P (ret, NULL);904 TEST_EQ_P (ret, NULL);
933
934 TEST_EQ (len, 2);
935 TEST_ALLOC_PARENT (env[0], env);
936 TEST_ALLOC_SIZE (env[0], 8);
937 TEST_EQ_STR (env[0], "FOO=BAR");
938
939 TEST_ALLOC_PARENT (env[1], env);
940 TEST_ALLOC_SIZE (env[1], 8);
941 TEST_EQ_STR (env[1], "BAZ=QUX");
942
943 TEST_EQ_P (env[2], NULL);
944
945 nih_free (env);905 nih_free (env);
946 continue;906 continue;
947 }907 }
@@ -954,6 +914,7 @@
954 TEST_EQ_STR (env[0], "FOO=BAR");914 TEST_EQ_STR (env[0], "FOO=BAR");
955915
956 TEST_EQ_P (env[1], NULL);916 TEST_EQ_P (env[1], NULL);
917 TEST_FREE (removed);
957918
958 nih_free (env);919 nih_free (env);
959 }920 }
@@ -991,6 +952,9 @@
991 /* Should have been expanded */952 /* Should have been expanded */
992 TEST_EQ_STR (env[1], "UPSTART_TEST_VARIABLE=foo");953 TEST_EQ_STR (env[1], "UPSTART_TEST_VARIABLE=foo");
993954
955 removed = env[1];
956 TEST_FREE_TAG (removed);
957
994 TEST_EQ_P (env[2], NULL);958 TEST_EQ_P (env[2], NULL);
995 }959 }
996960
@@ -999,18 +963,6 @@
999963
1000 if (test_alloc_failed) {964 if (test_alloc_failed) {
1001 TEST_EQ_P (ret, NULL);965 TEST_EQ_P (ret, NULL);
1002
1003 TEST_EQ (len, 2);
1004 TEST_ALLOC_PARENT (env[0], env);
1005 TEST_ALLOC_SIZE (env[0], 8);
1006 TEST_EQ_STR (env[0], "FOO=BAR");
1007
1008 TEST_ALLOC_PARENT (env[1], env);
1009 TEST_ALLOC_SIZE (env[1], 8);
1010 TEST_EQ_STR (env[1], "UPSTART_TEST_VARIABLE=foo");
1011
1012 TEST_EQ_P (env[2], NULL);
1013
1014 nih_free (env);966 nih_free (env);
1015 continue;967 continue;
1016 }968 }
@@ -1023,6 +975,7 @@
1023 TEST_EQ_STR (env[0], "FOO=BAR");975 TEST_EQ_STR (env[0], "FOO=BAR");
1024976
1025 TEST_EQ_P (env[1], NULL);977 TEST_EQ_P (env[1], NULL);
978 TEST_FREE (removed);
1026979
1027 nih_free (env);980 nih_free (env);
1028 }981 }

Subscribers

People subscribed via source and target branches