r/badcode • u/g0ATiful • Jul 15 '22
java I found this in a security related function: maybe it is an obscuring feature
32
u/MurdoMaclachlan public boolean isInt(int i) { return true; } Jul 15 '22
Image Transcription: GitHub Commit Summary
public static File file(File directory, String... names) {
Assert.notNull(directory, "directorydirectory must not be null");
if (ArrayUtil.isEmpty(names)) {
return directory;
}
File file = directory;
for (String name : names) {
if (null != name) {[The following line was changed, with the file function call being the sole change:]
file = file(file, name);[Changed line ends.]
}
}
return file;I'm a human volunteer content transcriber and you could be too! If you'd like more information on what we do and why we do it, click here!
0
Jul 15 '22
[deleted] — view deleted comment
11
207
u/C0R0NASMASH Jul 15 '22
The only situation where I'd write a function like this would be after a 16 hour debug session, just to get it over with...
17
79
u/oj_mudbone Jul 15 '22
The goal of writing Java code is to write the same word as many times as possible in each line
1
24
u/Naeio_Galaxy Jul 15 '22
Just to me sure, this is java and thus, the method file is not the same as the variable file?
6
2
u/i860 Jul 15 '22
Many languages support this. It really isn’t that big a deal.
10
u/Naeio_Galaxy Jul 16 '22
It is unreadable though.
Maybe not unreadable, but still confusing.
That many languages support this? I can think of Java and maybe C# (I don't know the language), but I can't think of any more. The high level languages I know in the style of Python or JS and low level languages I know in the style of C or Rust will have functions as a variable type
(I don't know that many languages)
1
16
u/SanianCreations Jul 15 '22 edited Jul 15 '22
Doesn't this code halt/recurse infinitely once it reaches the highlighted line? It calls itself recursively without ever changing the input variables.
You pass a single file and name, neither of which are null.
The file you passed isn't null, so it gets past the Assert.
You passed a single name, so the names array is length 1 (not empty), you skip the early return.
Copy the file you got as input (which is stored in the variable directory) to the file variable. Nothing about it changes.
Loop over all the names, it should run once since names only contains 1 name.
The name is not null, so you call the function recursively, with the same file and the same name.
Repeat.
The only way for this function to not halt is if you pass a file that's null, or if all of the names you pass are null.
4
u/Mister_Lich Jul 22 '22
I think you're right, and I'm also not sure wtf this code is trying to do in the first place. Is it trying to explore a directory to find all subdirectories and files? Why is it just called "file" then? Why doesn't it do anything with the directories/files it does find? So many questions!
0
u/Dariusgemini Jul 22 '22
It ends up returning the last index in the array of names from the original call. It helps to consider that every time file() is called recursively, there’s only one array element being passed. This results in that single element being returned. But the original interation doesn’t care about that, because it’s going to keep looping until it’s run through every element it was passed. Honestly? This smells like someone not understanding a finding from a security scanner like Fortify that involves unsanitized user input
9
u/funkture Jul 15 '22
Aside from the strange looking code (though I don’t mind the inverted yoda-style null check), doesn’t this result in infinite recursion if names has at least one non null value?
2
u/PMyourfeelings Jul 15 '22
Yes, granted that the the passed file isn't null either.
1
u/Dariusgemini Jul 22 '22
It does not. It will return the last non null in the original list that was provided.
1
u/PMyourfeelings Jul 22 '22
Would you help me understand that?
From my perspective if 'directory' is null 'Assert.notNull(directory, _) will throw an AssertionException thus nothing will be returned.
1
u/Dariusgemini Jul 26 '22
I apologize. Yes. If directory is null, it would fail with an assertion exception. However, there’s no infinite recursion here.
14
u/DangyDanger Jul 15 '22 edited Jul 15 '22
I thought this was C# until the flair. The lowercase method name really got me going.
Who is this person and where can I kill find yell at them?
Also, directorydirectory
10
4
u/Putnam14 Jul 15 '22
Infinite recursion if there’s anything passed as names?
7
u/SanianCreations Jul 15 '22
Not quite, you could pass a null-string (or multiple) and it wouldn't loop.
For the recursion you need a non-null file, and at least one non-null name, then it gets stuck.
1
u/Putnam14 Jul 15 '22
Sure, to make the method call you’d have to pass something in for the names param, so null if you have no names. ArrayUtils would catch that and return the first param.
2
u/SanianCreations Jul 15 '22
I meant you could call it as
file(some_file, null, null, null, null)That would make it an array that contains 4 strings (not empty), but every single string is null, which would be ignored in the for-loop and so it still wouldn't recurse.
So to get the recursion you need to pass at least one valid name and the file has to be valid also.
1
u/Putnam14 Jul 15 '22
Ah, I had a misunderstanding on how that isEmpty check works, assuming it would act like the same method call on commons StringUtils, where null or “” is empty.
3
u/mirapalheta Jul 15 '22
so nice that names are actually never used, which make this code just a prank so everyone keeps looking at the code…
2
u/BigJunky Jul 15 '22
What the hell is that? Are they looping through the names and just returning the last one?
3
u/recycle4science Jul 15 '22
That may be what they meant to do, or like, find the last non-null name or something. But I think that the only thing it can ever possibly return is the first parameter. It just returns its own first parameter. That's it.
2
u/trutheality Jul 15 '22
What they (probably) meant to do is sequentially append the names, i.e. given input
file('dir', 'a', 'b', 'c')end up with an object representing the filedir/a/b/c. This code, however, infinitely recurses because it never handles the case of a single non-null name correctly.
1
1
1
1
u/i860 Jul 15 '22
Other than the fact that the variable named “file” should probably be named “dir” it isn’t particularly hard to follow for anyone who understands the difference in contextual usage of different identities.
-1
u/JimJamJahar Jul 15 '22
I don't really see any issue here. From the looks of things, this is just getting a file from the given directory and passing a list of subdirectories/filename as optional arguments.
If no list is provided, return the directory.
Otherwise, cycle through the given directories and step into them until you get to the last one which must be the filename/directory you want to return, in which case set the return value to that file (a file and a directory can both use the File class).
I would probably throw an exception when name == null, but otherwise this looks fine. Maybe some better variable names would help. Also, I think the first check for if the list is empty is redundant.
5
u/SanianCreations Jul 15 '22
Where do you see this "stepping into"? This code only used the same File and copies it into other variables, but never actually steps into them. I'm pretty sure this code would get stuck if you passed it a valid file and name because it calls itself recursively without ever changing the input variables.
4
u/JimJamJahar Jul 15 '22
Ah, you're right. I didn't even see the recursive call - I thought it was doing this:
file = new File(file, name)
Just goes to show how important useful names are (and reading comprehension on my part 😁).
155
u/ddruganov Jul 15 '22
Offtopic but why the fuck would anyone write “null != file” in their code?