* Missing NULL check
@ 2023-09-12 15:40 jacob navia
2023-09-12 18:50 ` Torbjorn SVENSSON
2023-09-13 6:50 ` Alan Modra
0 siblings, 2 replies; 6+ messages in thread
From: jacob navia @ 2023-09-12 15:40 UTC (permalink / raw)
To: binutils
[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]
This is very similar to the last one:
FILE: elf-attrs.c line 232
FUNCTION: elf_new_obj_attr
/* Allocate/find an object attribute. */
static obj_attribute *
elf_new_obj_attr (bfd *abfd, int vendor, unsigned int tag)
{
obj_attribute *attr;
obj_attribute_list *list;
obj_attribute_list *p;
obj_attribute_list **lastp;
if (tag < NUM_KNOWN_OBJ_ATTRIBUTES)
{
/* Known tags are preallocated. */
attr = &elf_known_obj_attributes (abfd)[vendor][tag];
}
else
{
/* Create a new tag. */
list = (obj_attribute_list *)
bfd_alloc (abfd, sizeof (obj_attribute_list));
memset (list, 0, sizeof (obj_attribute_list));
list->tag = tag;
/* Keep the tag list in order. */
lastp = &elf_other_obj_attributes (abfd)[vendor];
for (p = *lastp; p; p = p->next)
{
if (tag < p->tag)
break;
lastp = &p->next;
}
list->next = *lastp;
*lastp = list;
attr = &list->attr;
}
return attr;
}
bfd_alloc can return NULL. This is not checked
How to fix:
Add
if (list == NULL) return NULL;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Missing NULL check 2023-09-12 15:40 Missing NULL check jacob navia @ 2023-09-12 18:50 ` Torbjorn SVENSSON 2023-09-12 19:34 ` Fangrui Song [not found] ` <DS7PR12MB5765DD3A03E4B143B950C3D6CBF1A@DS7PR12MB5765.namprd12.prod.outlook.com> 2023-09-13 6:50 ` Alan Modra 1 sibling, 2 replies; 6+ messages in thread From: Torbjorn SVENSSON @ 2023-09-12 18:50 UTC (permalink / raw) To: jacob navia, binutils On 2023-09-12 17:40, jacob navia wrote: > This is very similar to the last one: > FILE: elf-attrs.c line 232 > FUNCTION: elf_new_obj_attr > /* Allocate/find an object attribute. */ > static obj_attribute * > elf_new_obj_attr (bfd *abfd, int vendor, unsigned int tag) > { > obj_attribute *attr; > obj_attribute_list *list; > obj_attribute_list *p; > obj_attribute_list **lastp; > > > if (tag < NUM_KNOWN_OBJ_ATTRIBUTES) > { > /* Known tags are preallocated. */ > attr = &elf_known_obj_attributes (abfd)[vendor][tag]; > } > else > { > /* Create a new tag. */ > list = (obj_attribute_list *) > bfd_alloc (abfd, sizeof (obj_attribute_list)); > memset (list, 0, sizeof (obj_attribute_list)); > list->tag = tag; > /* Keep the tag list in order. */ > lastp = &elf_other_obj_attributes (abfd)[vendor]; > for (p = *lastp; p; p = p->next) > { > if (tag < p->tag) > break; > lastp = &p->next; > } > list->next = *lastp; > *lastp = list; > attr = &list->attr; > } > > return attr; > } > > bfd_alloc can return NULL. This is not checked > How to fix: > Add > if (list == NULL) return NULL; I've seen that you've sent a few mails similar to this one now. First of all, thanks for trying to improve the quality on binutils! If you want to help binutils even more, I would suggest that you create a proper patch and submit that instead of a mail with the above content as it would make it less work for a maintainer to actually commit your fix. Note, the above is just my personal opinion and might not be align with the preferences of the binutils maintainers. Kind regards, Torbjörn ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Missing NULL check 2023-09-12 18:50 ` Torbjorn SVENSSON @ 2023-09-12 19:34 ` Fangrui Song 2023-09-13 10:57 ` Nick Clifton [not found] ` <DS7PR12MB5765DD3A03E4B143B950C3D6CBF1A@DS7PR12MB5765.namprd12.prod.outlook.com> 1 sibling, 1 reply; 6+ messages in thread From: Fangrui Song @ 2023-09-12 19:34 UTC (permalink / raw) To: jacob navia; +Cc: Torbjorn SVENSSON, binutils On Tue, Sep 12, 2023 at 11:50 AM Torbjorn SVENSSON via Binutils <binutils@sourceware.org> wrote: > > > > On 2023-09-12 17:40, jacob navia wrote: > > This is very similar to the last one: > > FILE: elf-attrs.c line 232 > > FUNCTION: elf_new_obj_attr > > /* Allocate/find an object attribute. */ > > static obj_attribute * > > elf_new_obj_attr (bfd *abfd, int vendor, unsigned int tag) > > { > > obj_attribute *attr; > > obj_attribute_list *list; > > obj_attribute_list *p; > > obj_attribute_list **lastp; > > > > > > if (tag < NUM_KNOWN_OBJ_ATTRIBUTES) > > { > > /* Known tags are preallocated. */ > > attr = &elf_known_obj_attributes (abfd)[vendor][tag]; > > } > > else > > { > > /* Create a new tag. */ > > list = (obj_attribute_list *) > > bfd_alloc (abfd, sizeof (obj_attribute_list)); > > memset (list, 0, sizeof (obj_attribute_list)); > > list->tag = tag; > > /* Keep the tag list in order. */ > > lastp = &elf_other_obj_attributes (abfd)[vendor]; > > for (p = *lastp; p; p = p->next) > > { > > if (tag < p->tag) > > break; > > lastp = &p->next; > > } > > list->next = *lastp; > > *lastp = list; > > attr = &list->attr; > > } > > > > return attr; > > } > > > > bfd_alloc can return NULL. This is not checked > > How to fix: > > Add > > if (list == NULL) return NULL; > > I've seen that you've sent a few mails similar to this one now. > First of all, thanks for trying to improve the quality on binutils! > > If you want to help binutils even more, I would suggest that you create > a proper patch and submit that instead of a mail with the above content > as it would make it less work for a maintainer to actually commit your fix. > > Note, the above is just my personal opinion and might not be align with > the preferences of the binutils maintainers. > > Kind regards, > Torbjörn I share the same opinion as Torbjorn's. You might consider configuring `git send-email` with a MTA (e.g. msmtp), then you can send [PATCH] messages that a maintainer can apply with ease: b4 am 'https://inbox.sourceware.org/binutils/DB9PR08MB647413BBA3340351A802035F81F1A@DB9PR08MB6474.eurprd08.prod.outlook.com/T/#t' -o - | git am (See https://josefbacik.github.io/kernel/2021/10/18/lei-and-b4.html) If a patch is straightforward enough and a maintainer notices it, I am sure this will make it faster for a fix to be applied. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Missing NULL check 2023-09-12 19:34 ` Fangrui Song @ 2023-09-13 10:57 ` Nick Clifton 0 siblings, 0 replies; 6+ messages in thread From: Nick Clifton @ 2023-09-13 10:57 UTC (permalink / raw) To: jacob navia; +Cc: binutils Hi Jacob, Following on from Torbjorn's and Fanguri's points, I would also like to say that for non-trial bugs, or code improvements, it really helps if you file a report via the sourceware bugzilla system: https://sourceware.org/bugzilla/enter_bug.cgi?product=binutils This allows us to keep all of the discussion related to the issue in one place, and to refer to it in the code, git-log entries and changelog entries via a simple problem report number. Cheers Nick ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <DS7PR12MB5765DD3A03E4B143B950C3D6CBF1A@DS7PR12MB5765.namprd12.prod.outlook.com>]
* Re: Missing NULL check [not found] ` <DS7PR12MB5765DD3A03E4B143B950C3D6CBF1A@DS7PR12MB5765.namprd12.prod.outlook.com> @ 2023-09-13 14:28 ` jacob navia 0 siblings, 0 replies; 6+ messages in thread From: jacob navia @ 2023-09-13 14:28 UTC (permalink / raw) To: Fangrui Song; +Cc: Torbjorn SVENSSON, binutils [-- Attachment #1: Type: text/plain, Size: 7014 bytes --] > > I share the same opinion as Torbjorn's. > > You might consider configuring `git send-email` with a MTA (e.g. > msmtp), then you can send [PATCH] messages that a maintainer can apply > with ease: > > b4 am 'https://inbox.sourceware.org/binutils/DB9PR08MB647413BBA3340351A802035F81F1A@DB9PR08MB6474.eurprd08.prod.outlook.com/T/#t' > -o - | git am > > (See https://josefbacik.github.io/kernel/2021/10/18/lei-and-b4.html) > > If a patch is straightforward enough and a maintainer notices it, I am > sure this will make it faster for a fix to be applied. This looks easy for linux wizards… I am not. I am just trying to help. 1) I downloaded and installed smtp 2) Download and installed the « b4 » thing. 3) Downloaded and installed a fresh copy of binutils-gdb 4) Made the change for fixing missing NULL tests. 5) Committed my changes to git 6) After a lot of fiddling I did the command that you sent me: b4 am 'https://inbox.sourceware.org/binutils/DB9PR08MB647413BBA3340351A802035F81F1A@DB9PR08MB6474.eurprd08.prod.outlook.com/T/#t' -o - | git am It was splitter somewhere, did not work the first few times until I got that right. 7) OUTPUT: sipeed@lpi4a:~/binutils-gdb/bfd$ b4 am 'https://inbox.sourceware.org/binutils/DB9PR08MB647413BBA3340351A802035F81F1A@DB9PR08MB6474.eurprd08.prod.outlook.com/T/#t' -o - | git am Looking up https://inbox.sourceware.org/binutils/DB9PR08MB647413BBA3340351A802035F81F1A%40DB9PR08MB6474.eurprd08.prod.outlook.com Grabbing thread from inbox.sourceware.org/binutils/DB9PR08MB647413BBA3340351A802035F81F1A%40DB9PR08MB6474.eurprd08.prod.outlook.com/t.mbox.gz Traceback (most recent call last): File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 704, in urlopen httplib_response = self._make_request( ^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 450, in _make_request six.raise_from(e, None) File "<string>", line 3, in raise_from File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 445, in _make_request httplib_response = conn.getresponse() ^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/http/client.py", line 1378, in getresponse response.begin() File "/usr/lib/python3.11/http/client.py", line 318, in begin version, status, reason = self._read_status() ^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/http/client.py", line 287, in _read_status raise RemoteDisconnected("Remote end closed connection without" http.client.RemoteDisconnected: Remote end closed connection without response During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/usr/lib/python3/dist-packages/requests/adapters.py", line 489, in send resp = conn.urlopen( ^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 788, in urlopen retries = retries.increment( ^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/urllib3/util/retry.py", line 550, in increment raise six.reraise(type(error), error, _stacktrace) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/six.py", line 718, in reraise raise value.with_traceback(tb) File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 704, in urlopen httplib_response = self._make_request( ^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 450, in _make_request six.raise_from(e, None) File "<string>", line 3, in raise_from File "/usr/lib/python3/dist-packages/urllib3/connectionpool.py", line 445, in _make_request httplib_response = conn.getresponse() ^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/http/client.py", line 1378, in getresponse response.begin() File "/usr/lib/python3.11/http/client.py", line 318, in begin version, status, reason = self._read_status() ^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/http/client.py", line 287, in _read_status raise RemoteDisconnected("Remote end closed connection without" urllib3.exceptions.ProtocolError: ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response')) During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/usr/bin/b4", line 33, in <module> sys.exit(load_entry_point('b4==0.12.2', 'console_scripts', 'b4')()) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/b4/command.py", line 360, in cmd cmdargs.func(cmdargs) File "/usr/lib/python3/dist-packages/b4/command.py", line 91, in cmd_am b4.mbox.main(cmdargs) File "/usr/lib/python3/dist-packages/b4/mbox.py", line 694, in main msgid, msgs = b4.retrieve_messages(cmdargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/b4/__init__.py", line 3538, in retrieve_messages msgs = get_pi_thread_by_msgid(msgid, nocache=cmdargs.nocache, onlymsgids=pickings) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/b4/__init__.py", line 2843, in get_pi_thread_by_msgid msgs = get_pi_thread_by_url(t_mbx_url, nocache=nocache) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/b4/__init__.py", line 2800, in get_pi_thread_by_url resp = session.get(t_mbx_url) ^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/requests/sessions.py", line 600, in get return self.request("GET", url, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/requests/sessions.py", line 587, in request resp = self.send(prep, **send_kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/requests/sessions.py", line 701, in send r = adapter.send(request, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3/dist-packages/requests/adapters.py", line 547, in send raise ConnectionError(err, request=request) requests.exceptions.ConnectionError: ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response')) sipeed@lpi4a:~/binutils-gdb/bfd$ Look, I am not a linux wizard, nor a python wizard, and neither a git wizard. From the first lines the command seem to work since it says: Grabbing thread from inbox.sourceware.org/binutils/DB9PR08MB647413BBA3340351A802035F81F1A%40DB9PR08MB6474.eurprd08.prod.outlook.com/t.mbox.gz <http://inbox.sourceware.org/binutils/DB9PR08MB647413BBA3340351A802035F81F1A%40DB9PR08MB6474.eurprd08.prod.outlook.com/t.mbox.gz> Afterwards, please debug that yourself… ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Missing NULL check 2023-09-12 15:40 Missing NULL check jacob navia 2023-09-12 18:50 ` Torbjorn SVENSSON @ 2023-09-13 6:50 ` Alan Modra 1 sibling, 0 replies; 6+ messages in thread From: Alan Modra @ 2023-09-13 6:50 UTC (permalink / raw) To: jacob navia; +Cc: binutils On Tue, Sep 12, 2023 at 05:40:45PM +0200, jacob navia wrote: > Add > if (list == NULL) return NULL; Yes, we should be testing for a NULL return from bfd_alloc, but error handling then needs to be propagated up the call chain. If elf_new_obj_attr returns NULL then all its callers will segfault unless they are changed too. We also don't want to hide allocation failures which means functions like bfd_elf_add_obj_attr_int should return the obj_attribute pointer rather than void, and all callers should check the return value. I'll write a patch to do that. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-13 14:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-12 15:40 Missing NULL check jacob navia
2023-09-12 18:50 ` Torbjorn SVENSSON
2023-09-12 19:34 ` Fangrui Song
2023-09-13 10:57 ` Nick Clifton
[not found] ` <DS7PR12MB5765DD3A03E4B143B950C3D6CBF1A@DS7PR12MB5765.namprd12.prod.outlook.com>
2023-09-13 14:28 ` jacob navia
2023-09-13 6:50 ` Alan Modra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).