| Message ID | 20230725220056.25560-3-jorge.lopez2@hp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | hp-bioscfg: Address memory leaks and uninitialized variable errors | expand |
I went through this pretty carefully. There is a small bug in the
ORD_LIST_ELEMENTS case where the value_len is wrong. Otherwise, I
complained a little about style nits... You can feel free to ignore
everything except the ORD_LIST_ELEMENTS stuff.
regards,
dan carpenter
drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
129 static int hp_populate_ordered_list_elements_from_package(union acpi_object *order_obj,
130 int order_obj_count,
131 int instance_id)
132 {
133 char *str_value = NULL;
It would be better to make str_value local to the loop. Then we
could delete all the str_value = NULL assignments and the exit_list:
code at the end. At the end of the loop we could do something like:
kfree(str_value);
if (ret)
return ret;
134 int value_len = 0;
135 int ret;
136 u32 size;
137 u32 int_value = 0;
It confused me that it's called int_value when it's not an int. Just
call it "u32 value = 0;".
138 int elem;
139 int reqs;
140 int eloc;
141 char *tmpstr = NULL;
142 char *part_tmp = NULL;
143 int tmp_len = 0;
144 char *part = NULL;
145 struct ordered_list_data *ordered_list_data = &bioscfg_drv.ordered_list_data[instance_id];
146
147 if (!order_obj)
148 return -EINVAL;
149
150 for (elem = 1, eloc = 1; elem < order_obj_count; elem++, eloc++) {
151 /* ONLY look at the first ORDERED_ELEM_CNT elements */
152 if (eloc == ORD_ELEM_CNT)
153 goto exit_list;
We really expect to exit when eloc is ORD_ELEM_CNT. So I think this
would be more clear as:
for (elem = 1, eloc = 1; eloc < ORD_ELEM_CNT; elem++, eloc++) {
I don't really know what eloc stands for... dst_idx?
if (elem >= order_obj_count)
return -EINVAL;
obj = &order_obj[elem];
Let's move the "Check that both expected and read object type match"
stuff from line 173 up to here.
if (obj->type != expected_order_types[eloc]) {
pr_err("Error expected type %d for elem %d, but got type %d instead\n",
expected_order_types[eloc], elem, obj->type);
return -EINVAL;
}
154
155 switch (order_obj[elem].type) {
switch(obj->type) {
156 case ACPI_TYPE_STRING:
157 if (elem != PREREQUISITES && elem != ORD_LIST_ELEMENTS) {
158 ret = hp_convert_hexstr_to_str(order_obj[elem].string.pointer,
159 order_obj[elem].string.length,
160 &str_value, &value_len);
161 if (ret)
162 continue;
return ret;
163 }
164 break;
165 case ACPI_TYPE_INTEGER:
166 int_value = (u32)order_obj[elem].integer.value;
167 break;
168 default:
169 pr_warn("Unsupported object type [%d]\n", order_obj[elem].type);
170 continue;
return -EINVAL;
171 }
172
173 /* Check that both expected and read object type match */
174 if (expected_order_types[eloc] != order_obj[elem].type) {
175 pr_err("Error expected type %d for elem %d, but got type %d instead\n",
176 expected_order_types[eloc], elem, order_obj[elem].type);
177 kfree(str_value);
178 return -EIO;
179 }
180
181 /* Assign appropriate element value to corresponding field*/
182 switch (eloc) {
183 case VALUE:
184 strscpy(ordered_list_data->current_value,
185 str_value, sizeof(ordered_list_data->current_value));
186 replace_char_str(ordered_list_data->current_value, COMMA_SEP, SEMICOLON_SEP);
187 break;
188 case PATH:
189 strscpy(ordered_list_data->common.path, str_value,
190 sizeof(ordered_list_data->common.path));
191 break;
192 case IS_READONLY:
193 ordered_list_data->common.is_readonly = int_value;
194 break;
195 case DISPLAY_IN_UI:
196 ordered_list_data->common.display_in_ui = int_value;
197 break;
198 case REQUIRES_PHYSICAL_PRESENCE:
199 ordered_list_data->common.requires_physical_presence = int_value;
200 break;
201 case SEQUENCE:
202 ordered_list_data->common.sequence = int_value;
203 break;
204 case PREREQUISITES_SIZE:
205 ordered_list_data->common.prerequisites_size = int_value;
206 if (int_value > MAX_PREREQUISITES_SIZE)
207 pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
ret = -EINVAL;
break;
208
209 /*
210 * This HACK is needed to keep the expected
211 * element list pointing to the right obj[elem].type
212 * when the size is zero. PREREQUISITES
213 * object is omitted by BIOS when the size is
214 * zero.
It's not really a HACK.
/*
* If prerequisites_size is zero then there isn't a PREREQUISITES
* object so skip that and jump to SECURITY_LEVEL.
*
*/
215 */
216 if (int_value == 0)
217 eloc++;
218 break;
219 case PREREQUISITES:
220 size = min_t(u32, ordered_list_data->common.prerequisites_size,
221 MAX_PREREQUISITES_SIZE);
If we returned when we hit invalid data then there is no need for this
min_t().
size = ordered_list_data->common.prerequisites_size;
222 for (reqs = 0; reqs < size; reqs++) {
223 ret = hp_convert_hexstr_to_str(order_obj[elem + reqs].string.pointer,
224 order_obj[elem + reqs].string.length,
225 &str_value, &value_len);
This is fine but it might be nicer to do what ORD_LIST_ELEMENTS does
and use tmpstr instead of str_value.
226
227 if (ret)
228 continue;
break;
229
230 strscpy(ordered_list_data->common.prerequisites[reqs],
231 str_value,
232 sizeof(ordered_list_data->common.prerequisites[reqs]));
233
234 kfree(str_value);
235 str_value = NULL;
236 }
237 break;
238
239 case SECURITY_LEVEL:
240 ordered_list_data->common.security_level = int_value;
241 break;
242
243 case ORD_LIST_SIZE:
244 ordered_list_data->elements_size = int_value;
245 if (int_value > MAX_ELEMENTS_SIZE)
246 pr_warn("Ordered List size value exceeded the maximum number of elements supported or data may be malformed\n");
ret = -EINVAL;
break;
247 /*
248 * This HACK is needed to keep the expected
249 * element list pointing to the right obj[elem].type
250 * when the size is zero. ORD_LIST_ELEMENTS
251 * object is omitted by BIOS when the size is
252 * zero.
253 */
254 if (int_value == 0)
255 eloc++;
256 break;
257 case ORD_LIST_ELEMENTS:
258 size = ordered_list_data->elements_size;
We don't use size anywhere so this can be deleted.
259
260 /*
261 * Ordered list data is stored in hex and comma separated format
262 * Convert the data and split it to show each element
263 */
264 ret = hp_convert_hexstr_to_str(str_value, value_len, &tmpstr, &tmp_len);
The value_len here is wrong. We don't read value_len for ORD_LIST_ELEMENTS
or PREREQUISITES so this value_len comes from the PATH object.
265 if (ret)
266 goto exit_list;
267
268 part_tmp = tmpstr;
269 part = strsep(&part_tmp, COMMA_SEP);
270 if (!part)
271 strscpy(ordered_list_data->elements[0],
272 tmpstr,
273 sizeof(ordered_list_data->elements[0]));
274
275 for (elem = 1; elem < MAX_ELEMENTS_SIZE && part; elem++) {
Please don't re-use the "elem" iterator for this inner loop.
regards,
dan carpenter
276 strscpy(ordered_list_data->elements[elem],
277 part,
278 sizeof(ordered_list_data->elements[elem]));
279 part = strsep(&part_tmp, SEMICOLON_SEP);
280 }
281
282 kfree(str_value);
283 str_value = NULL;
284 break;
285 default:
286 pr_warn("Invalid element: %d found in Ordered_List attribute or data may be malformed\n", elem);
287 break;
288 }
289 kfree(tmpstr);
290 tmpstr = NULL;
291 kfree(str_value);
292 str_value = NULL;
293 }
294
295 exit_list:
296 kfree(tmpstr);
297 kfree(str_value);
298 return 0;
299 }
On Thu, Jul 27, 2023 at 1:21 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > I went through this pretty carefully. There is a small bug in the > ORD_LIST_ELEMENTS case where the value_len is wrong. Otherwise, I > complained a little about style nits... You can feel free to ignore > everything except the ORD_LIST_ELEMENTS stuff. > > regards, > dan carpenter > > drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c > 129 static int hp_populate_ordered_list_elements_from_package(union acpi_object *order_obj, > 130 int order_obj_count, > 131 int instance_id) > 132 { > 133 char *str_value = NULL; > > It would be better to make str_value local to the loop. Then we > could delete all the str_value = NULL assignments and the exit_list: > code at the end. At the end of the loop we could do something like: > > kfree(str_value); > if (ret) > return ret; > > 134 int value_len = 0; > 135 int ret; > 136 u32 size; > 137 u32 int_value = 0; > > It confused me that it's called int_value when it's not an int. Just > call it "u32 value = 0;". The variable is named int_value when it is not an int because it stores the value reported by ACPI_TYPE_INTEGER package. The variable will be renamed to type_int_value; > > 138 int elem; > 139 int reqs; > 140 int eloc; > 141 char *tmpstr = NULL; > 142 char *part_tmp = NULL; > 143 int tmp_len = 0; > 144 char *part = NULL; > 145 struct ordered_list_data *ordered_list_data = &bioscfg_drv.ordered_list_data[instance_id]; > 146 > 147 if (!order_obj) > 148 return -EINVAL; > 149 > 150 for (elem = 1, eloc = 1; elem < order_obj_count; elem++, eloc++) { > 151 /* ONLY look at the first ORDERED_ELEM_CNT elements */ > 152 if (eloc == ORD_ELEM_CNT) > 153 goto exit_list; > > We really expect to exit when eloc is ORD_ELEM_CNT. So I think this > would be more clear as: > > for (elem = 1, eloc = 1; eloc < ORD_ELEM_CNT; elem++, eloc++) { > Done! > I don't really know what eloc stands for... dst_idx? > > if (elem >= order_obj_count) > return -EINVAL; > > obj = &order_obj[elem]; > 'eloc' stands for 'element location'. eloc helps keep track conditions such when ORD_LIST_SIZE and/or PREREQUISITES_SiZE value is zero. > Let's move the "Check that both expected and read object type match" > stuff from line 173 up to here. > > if (obj->type != expected_order_types[eloc]) { > pr_err("Error expected type %d for elem %d, but got type %d instead\n", > expected_order_types[eloc], elem, obj->type); > return -EINVAL; > } > > 154 > 155 switch (order_obj[elem].type) { > > switch(obj->type) { > > 156 case ACPI_TYPE_STRING: > 157 if (elem != PREREQUISITES && elem != ORD_LIST_ELEMENTS) { > 158 ret = hp_convert_hexstr_to_str(order_obj[elem].string.pointer, > 159 order_obj[elem].string.length, > 160 &str_value, &value_len); > 161 if (ret) > 162 continue; > > return ret; > > 163 } > 164 break; > 165 case ACPI_TYPE_INTEGER: > 166 int_value = (u32)order_obj[elem].integer.value; > 167 break; > 168 default: > 169 pr_warn("Unsupported object type [%d]\n", order_obj[elem].type); > 170 continue; > > return -EINVAL; We encountered during our testing that one or more packages could be assigned the wrong package type or invalid data.. For this reason, it was decided to ignore any invalid data and incorrect type package and allow the read process to continue. > > 171 } > 172 > 173 /* Check that both expected and read object type match */ > 174 if (expected_order_types[eloc] != order_obj[elem].type) { > 175 pr_err("Error expected type %d for elem %d, but got type %d instead\n", > 176 expected_order_types[eloc], elem, order_obj[elem].type); > 177 kfree(str_value); > 178 return -EIO; > 179 } > 180 > 181 /* Assign appropriate element value to corresponding field*/ > 182 switch (eloc) { > 183 case VALUE: > 184 strscpy(ordered_list_data->current_value, > 185 str_value, sizeof(ordered_list_data->current_value)); > 186 replace_char_str(ordered_list_data->current_value, COMMA_SEP, SEMICOLON_SEP); > 187 break; > 188 case PATH: > 189 strscpy(ordered_list_data->common.path, str_value, > 190 sizeof(ordered_list_data->common.path)); > 191 break; > 192 case IS_READONLY: > 193 ordered_list_data->common.is_readonly = int_value; > 194 break; > 195 case DISPLAY_IN_UI: > 196 ordered_list_data->common.display_in_ui = int_value; > 197 break; > 198 case REQUIRES_PHYSICAL_PRESENCE: > 199 ordered_list_data->common.requires_physical_presence = int_value; > 200 break; > 201 case SEQUENCE: > 202 ordered_list_data->common.sequence = int_value; > 203 break; > 204 case PREREQUISITES_SIZE: > 205 ordered_list_data->common.prerequisites_size = int_value; > 206 if (int_value > MAX_PREREQUISITES_SIZE) > 207 pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n"); > > ret = -EINVAL; > break; > We encountered during our testing that one or more packages could be assigned the wrong package type or invalid data.. For this reason, it was decided to ignore any invalid data and incorrect type package and allow the read process to continue. > 208 > 209 /* > 210 * This HACK is needed to keep the expected > 211 * element list pointing to the right obj[elem].type > 212 * when the size is zero. PREREQUISITES > 213 * object is omitted by BIOS when the size is > 214 * zero. > > It's not really a HACK. Will change the term HACK to 'step' > > /* > * If prerequisites_size is zero then there isn't a PREREQUISITES > * object so skip that and jump to SECURITY_LEVEL. > * > */ > > > 215 */ > 216 if (int_value == 0) > 217 eloc++; > 218 break; > 219 case PREREQUISITES: > 220 size = min_t(u32, ordered_list_data->common.prerequisites_size, > 221 MAX_PREREQUISITES_SIZE); > > If we returned when we hit invalid data then there is no need for this > min_t(). > > size = ordered_list_data->common.prerequisites_size; > > 222 for (reqs = 0; reqs < size; reqs++) { > 223 ret = hp_convert_hexstr_to_str(order_obj[elem + reqs].string.pointer, > 224 order_obj[elem + reqs].string.length, > 225 &str_value, &value_len); > > This is fine but it might be nicer to do what ORD_LIST_ELEMENTS does > and use tmpstr instead of str_value. > > 226 > 227 if (ret) > 228 continue; > > break; > > 229 > 230 strscpy(ordered_list_data->common.prerequisites[reqs], > 231 str_value, > 232 sizeof(ordered_list_data->common.prerequisites[reqs])); > 233 > 234 kfree(str_value); > 235 str_value = NULL; > 236 } > 237 break; > 238 > 239 case SECURITY_LEVEL: > 240 ordered_list_data->common.security_level = int_value; > 241 break; > 242 > 243 case ORD_LIST_SIZE: > 244 ordered_list_data->elements_size = int_value; > 245 if (int_value > MAX_ELEMENTS_SIZE) > 246 pr_warn("Ordered List size value exceeded the maximum number of elements supported or data may be malformed\n"); > > ret = -EINVAL; > break; > > 247 /* > 248 * This HACK is needed to keep the expected > 249 * element list pointing to the right obj[elem].type > 250 * when the size is zero. ORD_LIST_ELEMENTS > 251 * object is omitted by BIOS when the size is > 252 * zero. > 253 */ > 254 if (int_value == 0) > 255 eloc++; > 256 break; > 257 case ORD_LIST_ELEMENTS: > 258 size = ordered_list_data->elements_size; > > We don't use size anywhere so this can be deleted. > > 259 > 260 /* > 261 * Ordered list data is stored in hex and comma separated format > 262 * Convert the data and split it to show each element > 263 */ > 264 ret = hp_convert_hexstr_to_str(str_value, value_len, &tmpstr, &tmp_len); > > The value_len here is wrong. We don't read value_len for ORD_LIST_ELEMENTS > or PREREQUISITES so this value_len comes from the PATH object. The size > > 265 if (ret) > 266 goto exit_list; > 267 > 268 part_tmp = tmpstr; > 269 part = strsep(&part_tmp, COMMA_SEP); > 270 if (!part) > 271 strscpy(ordered_list_data->elements[0], > 272 tmpstr, > 273 sizeof(ordered_list_data->elements[0])); > 274 > 275 for (elem = 1; elem < MAX_ELEMENTS_SIZE && part; elem++) { > > Please don't re-use the "elem" iterator for this inner loop. Done! > > regards, > dan carpenter > > 276 strscpy(ordered_list_data->elements[elem], > 277 part, > 278 sizeof(ordered_list_data->elements[elem])); > 279 part = strsep(&part_tmp, SEMICOLON_SEP); > 280 } > 281 > 282 kfree(str_value); > 283 str_value = NULL; > 284 break; > 285 default: > 286 pr_warn("Invalid element: %d found in Ordered_List attribute or data may be malformed\n", elem); > 287 break; > 288 } > 289 kfree(tmpstr); > 290 tmpstr = NULL; > 291 kfree(str_value); > 292 str_value = NULL; > 293 } > 294 > 295 exit_list: > 296 kfree(tmpstr); > 297 kfree(str_value); > 298 return 0; > 299 } > >
On Mon, Jul 31, 2023 at 11:03:42AM -0500, Jorge Lopez wrote: > On Thu, Jul 27, 2023 at 1:21 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > 134 int value_len = 0; > > 135 int ret; > > 136 u32 size; > > 137 u32 int_value = 0; > > > > It confused me that it's called int_value when it's not an int. Just > > call it "u32 value = 0;". > > The variable is named int_value when it is not an int because it > stores the value reported by ACPI_TYPE_INTEGER package. > The variable will be renamed to type_int_value; Eep! That's even worse! Just leave it as-is then. > > 201 case SEQUENCE: > > 202 ordered_list_data->common.sequence = int_value; > > 203 break; > > 204 case PREREQUISITES_SIZE: > > 205 ordered_list_data->common.prerequisites_size = int_value; > > 206 if (int_value > MAX_PREREQUISITES_SIZE) > > 207 pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n"); > > > > ret = -EINVAL; > > break; > > > We encountered during our testing that one or more packages could be > assigned the wrong package type or invalid data.. > For this reason, it was decided to ignore any invalid data and > incorrect type package and allow the read process to continue. > So you have BIOSes which are still printing this warning and you can't fix it? Fine. Are you sure it's not because you re-used the elem iterator and started looping again in the middle? Could you at least do the bounds check here instead of in the next step? if (int_value > MAX_PREREQUISITES_SIZE) { pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n"); int_value = MAX_PREREQUISITES_SIZE; } ordered_list_data->common.prerequisites_size = int_value; > > 257 case ORD_LIST_ELEMENTS: > > 258 size = ordered_list_data->elements_size; > > > > We don't use size anywhere so this can be deleted. > > > > 259 > > 260 /* > > 261 * Ordered list data is stored in hex and comma separated format > > 262 * Convert the data and split it to show each element > > 263 */ > > 264 ret = hp_convert_hexstr_to_str(str_value, value_len, &tmpstr, &tmp_len); > > > > The value_len here is wrong. We don't read value_len for ORD_LIST_ELEMENTS > > or PREREQUISITES so this value_len comes from the PATH object. > The size ? regards, dan carpenter
On Mon, Jul 31, 2023 at 11:16 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Mon, Jul 31, 2023 at 11:03:42AM -0500, Jorge Lopez wrote: > > On Thu, Jul 27, 2023 at 1:21 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > 134 int value_len = 0; > > > 135 int ret; > > > 136 u32 size; > > > 137 u32 int_value = 0; > > > > > > It confused me that it's called int_value when it's not an int. Just > > > call it "u32 value = 0;". > > > > The variable is named int_value when it is not an int because it > > stores the value reported by ACPI_TYPE_INTEGER package. > > The variable will be renamed to type_int_value; > > Eep! That's even worse! Just leave it as-is then. Oops! just send a new patch using type_int_value. Should I change it back? > > > > 201 case SEQUENCE: > > > 202 ordered_list_data->common.sequence = int_value; > > > 203 break; > > > 204 case PREREQUISITES_SIZE: > > > 205 ordered_list_data->common.prerequisites_size = int_value; > > > 206 if (int_value > MAX_PREREQUISITES_SIZE) > > > 207 pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n"); > > > > > > ret = -EINVAL; > > > break; > > > > > We encountered during our testing that one or more packages could be > > assigned the wrong package type or invalid data.. > > For this reason, it was decided to ignore any invalid data and > > incorrect type package and allow the read process to continue. > > > > So you have BIOSes which are still printing this warning and you can't > fix it? Fine. Are you sure it's not because you re-used the elem > iterator and started looping again in the middle? Yes. I am sure. The BIOS where this code is applicable to is for 2018 platforms and earlier. This is the reason, a customer may encounter this problem in an old BIOS. > > Could you at least do the bounds check here instead of in the next step? > > > if (int_value > MAX_PREREQUISITES_SIZE) { > pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n"); > int_value = MAX_PREREQUISITES_SIZE; > } > ordered_list_data->common.prerequisites_size = int_value; > > > > 257 case ORD_LIST_ELEMENTS: > > > 258 size = ordered_list_data->elements_size; > > > > > > We don't use size anywhere so this can be deleted. > > > > > > 259 > > > 260 /* > > > 261 * Ordered list data is stored in hex and comma separated format > > > 262 * Convert the data and split it to show each element > > > 263 */ > > > 264 ret = hp_convert_hexstr_to_str(str_value, value_len, &tmpstr, &tmp_len); > > > > > > The value_len here is wrong. We don't read value_len for ORD_LIST_ELEMENTS > > > or PREREQUISITES so this value_len comes from the PATH object. > > The size > I meant to say, I should have used 'size' instead of 'value_len' in line 264. ret = hp_convert_hexstr_to_str(str_value, size, &tmpstr, &tmp_len); > ? > > regards, > dan carpenter
On Mon, Jul 31, 2023 at 11:35:35AM -0500, Jorge Lopez wrote: > On Mon, Jul 31, 2023 at 11:16 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > On Mon, Jul 31, 2023 at 11:03:42AM -0500, Jorge Lopez wrote: > > > On Thu, Jul 27, 2023 at 1:21 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > 134 int value_len = 0; > > > > 135 int ret; > > > > 136 u32 size; > > > > 137 u32 int_value = 0; > > > > > > > > It confused me that it's called int_value when it's not an int. Just > > > > call it "u32 value = 0;". > > > > > > The variable is named int_value when it is not an int because it > > > stores the value reported by ACPI_TYPE_INTEGER package. > > > The variable will be renamed to type_int_value; > > > > Eep! That's even worse! Just leave it as-is then. > > Oops! just send a new patch using type_int_value. Should I change it back? In order of preference for me, it's "value", "int_value", and "type_int_value". But it doesn't really matter. I feel there were a couple bugs like the size vs value_len and re-using the iterator. You have addressed the real problems so lets not worry about variable names. Whatever you pick is fine. regards, dan carpenter
I submitted a new set of patches as directed by Hans in which the variable name 'int_value' was kept. Thank you for replying back to me. Regards, Jorge Lopez On Tue, Aug 1, 2023 at 12:54 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Mon, Jul 31, 2023 at 11:35:35AM -0500, Jorge Lopez wrote: > > On Mon, Jul 31, 2023 at 11:16 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > > > On Mon, Jul 31, 2023 at 11:03:42AM -0500, Jorge Lopez wrote: > > > > On Thu, Jul 27, 2023 at 1:21 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > > 134 int value_len = 0; > > > > > 135 int ret; > > > > > 136 u32 size; > > > > > 137 u32 int_value = 0; > > > > > > > > > > It confused me that it's called int_value when it's not an int. Just > > > > > call it "u32 value = 0;". > > > > > > > > The variable is named int_value when it is not an int because it > > > > stores the value reported by ACPI_TYPE_INTEGER package. > > > > The variable will be renamed to type_int_value; > > > > > > Eep! That's even worse! Just leave it as-is then. > > > > Oops! just send a new patch using type_int_value. Should I change it back? > > In order of preference for me, it's "value", "int_value", and > "type_int_value". But it doesn't really matter. I feel there were a > couple bugs like the size vs value_len and re-using the iterator. You > have addressed the real problems so lets not worry about variable names. > Whatever you pick is fine. > > regards, > dan carpenter >
diff --git a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c index 7e49a8427c06..89e67db733eb 100644 --- a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c +++ b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c @@ -131,10 +131,10 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord int instance_id) { char *str_value = NULL; - int value_len; + int value_len = 0; int ret; u32 size; - u32 int_value; + u32 int_value = 0; int elem; int reqs; int eloc; @@ -174,6 +174,7 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord if (expected_order_types[eloc] != order_obj[elem].type) { pr_err("Error expected type %d for elem %d, but got type %d instead\n", expected_order_types[eloc], elem, order_obj[elem].type); + kfree(str_value); return -EIO; } @@ -231,6 +232,7 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord sizeof(ordered_list_data->common.prerequisites[reqs])); kfree(str_value); + str_value = NULL; } break; @@ -277,13 +279,17 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord part = strsep(&part_tmp, SEMICOLON_SEP); } + kfree(str_value); + str_value = NULL; break; default: pr_warn("Invalid element: %d found in Ordered_List attribute or data may be malformed\n", elem); break; } kfree(tmpstr); + tmpstr = NULL; kfree(str_value); + str_value = NULL; } exit_list:
Address memory leaks in hp_populate_ordered_list_elements_from_package() and uninitialized variable errors. Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com> --- Based on the latest platform-drivers-x86.git/for-next --- .../platform/x86/hp/hp-bioscfg/order-list-attributes.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)