Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v.in.dwg: Avoid using same variable as parameter and destination in sprintf #4262

Merged
merged 1 commit into from
Sep 15, 2024

Conversation

ymdatta
Copy link
Contributor

@ymdatta ymdatta commented Aug 30, 2024

Currently, one instance of sprintf has same variable as parameter and destination in sprintf. This scneario leads to undefined behavior in C.

sprintf(buf, _("%s Cannot open %s"), buf, path);

Modify the code to:

  1. Write initial error string using snprintf() onto the buffer. Using snprintf() makes sure that we stay within the buffer size and avoid overflow errors.
  2. Use snprintf() again to write another error string at the end of previous error string in the same buffer. We again use snprintf() to make sure we are not overflowing the buffer with data.

This was found using cppcheck tool.

Prior to fix:

image

After fix:

image

Additional information:

  1. Machine used: Virtual Machine running Ubuntu 22.04.4 LTS.
  2. Reproduction rate: 100%, reproducible every time.
  3. Architecture: Not specific to any underlying architecture.

@github-actions github-actions bot added vector Related to vector data processing C Related code is in C module labels Aug 30, 2024
@ymdatta
Copy link
Contributor Author

ymdatta commented Aug 30, 2024

Note: I couldn't compile the files in this package as I was not able to download the OpenDWG toolkit headers and libraries from http://www.opendwg.org from instructions in README. I believe the link is dead. So, I'm a bit uneasy about these changes. But, it's still a problem at hand as current code can cause undefined behavior.

I want to get inputs from you all on this.

@ymdatta ymdatta changed the title v.in.dwg: avoid src and dst overlap in sprintf v.in.dwg: Avoid src and dst overlap in sprintf Aug 30, 2024
vector/v.in.dwg/main.c Outdated Show resolved Hide resolved
@ymdatta ymdatta force-pushed the cppcheck_sprintfoverlappingdata branch from 03093da to 962be87 Compare August 31, 2024 00:12
vector/v.in.dwg/main.c Outdated Show resolved Hide resolved
Currently, one instance of sprintf has same variable as parameter
and destination in sprintf. This scneario leads to undefined
behavior in C.

Modify the code to:

1. Write initial error string using snprintf() onto the buffer.
   Using snprintf() makes sure that we stay within the buffer size
   and avoid overflow errors.

2. Use snprintf() again to write another error string at the end
   of previous error string in the same buffer. We again use
   snprintf() to make sure we are not overflowing the buffer with
   data.

Signed-off-by: Mohan Yelugoti <ymdatta.work@gmail.com>
@ymdatta ymdatta force-pushed the cppcheck_sprintfoverlappingdata branch from 962be87 to fc5982c Compare August 31, 2024 23:37
@ymdatta ymdatta changed the title v.in.dwg: Avoid src and dst overlap in sprintf v.in.dwg: Avoid using same variable as parameter and destination in sprintf Aug 31, 2024
@neteler
Copy link
Member

neteler commented Sep 2, 2024

I was not able to download the OpenDWG toolkit headers and libraries from http://www.opendwg.org from instructions in README. I believe the link is dead. So, I'm a bit uneasy about these changes

I have search around and the OpenDWG page went offline around 2011. Perhaps we should rather retire this code?

@nilason
Copy link
Contributor

nilason commented Sep 2, 2024

I was not able to download the OpenDWG toolkit headers and libraries from http://www.opendwg.org from instructions in README. I believe the link is dead. So, I'm a bit uneasy about these changes

I have search around and the OpenDWG page went offline around 2011. Perhaps we should rather retire this code?

I think we should. We have v.in.redwg as replacement.

@ymdatta
Copy link
Contributor Author

ymdatta commented Sep 2, 2024

That makes sense.

By retiring the code, do we stop pushing new commits to this module? If so, I can close this PR.

@wenzeslaus
Copy link
Member

@nilason if your comment was resolved, I suggest merging and then removing the disabled code in another PR. If it is not resolved, I suggest closing and, again, removing.

@nilason
Copy link
Contributor

nilason commented Sep 13, 2024

@nilason if your comment was resolved, I suggest merging and then removing the disabled code in another PR. If it is not resolved, I suggest closing and, again, removing.

@ymdatta did resolve the changes of the code and came up a nice string catenation solution using snprinft() (strlen() was the key).
However, if we are going to remove the module, why push any updates to it?

@neteler
Copy link
Member

neteler commented Sep 13, 2024

The update is appreciated and then we may move it to add-ons, in a good shape.

@nilason
Copy link
Contributor

nilason commented Sep 13, 2024

The update is appreciated and then we may move it to add-ons, in a good shape.

Move is something else than remove :-) In not sure about the good shape, this has probably not been tested (or even used) in quite a while (depending on proprietary SDK), although it will certainly be better!

@wenzeslaus
Copy link
Member

However, if we are going to remove the module, why push any updates to it?

Just because @ymdatta already created the changes. I would not be suggesting that otherwise.

I'm saying, let's merge this (rather than close), and then remove the code (directory and Makefile entry).

@echoix echoix merged commit 1a89301 into OSGeo:main Sep 15, 2024
26 of 28 checks passed
@neteler neteler added this to the 8.5.0 milestone Sep 16, 2024
@wenzeslaus
Copy link
Member

@ymdatta, can you please create a PR to remove v.in.dwg?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C module vector Related to vector data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants